proto definitions for settings service

[afbrock]

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Labels

Please add one or more of the following labels to categorize your PR:

• added: For new features.
• changed: For changes in existing functionality.
• deprecated: For soon-to-be-removed features.
• removed: For features being removed.
• fixed: For any bug fixed.
• security: In case of vulnerabilities

This is important to improve the readability of release notes.

Setup process

Screenshots

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

Docs link

• main
  • Flyte 2 #6583
    • proto definitions for settings service 👈

[wild-endeavor]

..

[wild-endeavor]

did we say we were getting rid of GPU?

[wild-endeavor]

limits are the max that can be specified right? can we call it a cap or a maximum? i'm worried the word limit is confusing in the context of k8s.

Also does it make sense to support GPU here @EngHabu ?

[EngHabu]

sure..

[afbrock]

so i think k8s has request/limit, I think if we want to make it match we should use that, but to me it is more intuitive to say min/max... except if we have scale down as a feature we might also start in the middle somewhere rather than at the minimum. my gut says we should say min/max because that is pretty clear, but that also isnt coming from a k8s background

[wild-endeavor]

I think we need a property bag here as well. i feel like that one is a generic struct @EngHabu? Or should it be property bags? dictionary of structs keyed off a string?

[EngHabu]

for "everything else" ?

[afbrock]

I think there is only one way that really makes sense - and that is if we are making these consumable by customers or 3rd parties where we do not use or do not have control or simply cannot implement them in our control plane. Adding an 'everything else' bucket basically means that the very next change someone makes to add a setting they need to answer the question 'does this get proper proto or does it go in the bag'. and then every consumer has to ask 'did that new setting get proper proto definition, or is it in the bag, and if it is in the bag where is its name defined'. and that mental work I really want to avoid.

[wild-endeavor]

chatted offline... let's ignore this for now and come to it when we need to work on cluster resource controller

[wild-endeavor]

weren't these originally going to be a oneof? Is there a benefit to making these separate objects (StringSetting, IntSetting, etc.). I understand you did this so that you can do

StringSetting raw_data_path = 1;

and then if someone receives a StorageSettings object, they'll know that it contains a string type... just wondering if this is a useful feature - in what scenario will this be useful to know above and beyond what the original oneof block would give you?

(see the SettingValue object in the md file)

[afbrock]

i am not that familiar with the oneofs - if someone makes a create request with a setting that is a oneof, can they put whatever type they want in there?

[wild-endeavor]

we should add a description to each setting also.

[wild-endeavor]

what is this for again? shouldn't it already be in each setting object?

[afbrock]

oh, yeah, let me change it so that it is in the settings wrapper

[wild-endeavor]

i don't think we should use string data here. This is the endpoint that the SDK CLI will probably use right? The sole purpose of this endpoint is so that changes to the settings object can be handled without forcing the user to upgrade. But I think a plain string is too loose right? I think we can assume that the SDK will always have access to the base setting value objects (be it SettingValue or the collection of StringSetting, IntSetting, etc.)... given that, shouldn't this be something like map<string, oneof stringsetting...> or map<string, SettingValue>?

Also, what was the plan for map/list types? the original plan was to have them be additive. We don't need to do that - if we just force the user to copy/paste any values from higher scopes that actually makes the editing experience simpler. (search for "Map-type settings should show each scope's contribution separately" in the md file to see what i mean) Is that the plan? just make the user do it?

I know i'm referencing the original doc... don't worry about the yaml formatting part in that doc please (we'll take care of the actual yaml parts), but the information does need to be in the api responses.

[afbrock]

the reason for it to be string data is for the comments. that may be unnecessary if we code into the sdk the comments about the level and description and which can be edited and which not, but if we wanted to add any other descriptive elements or anything like that this would allow it to be totally controlled by the back end and not the sdk.
That said, we can change it (and update) so that there is less parsing work involved by the backend.

[wild-endeavor]

i think we also need boolean

[wild-endeavor]

should we not just use SettingsRecord here?

[wild-endeavor]

Can this also be SettingsRecord

[wild-endeavor]

ditto

[wild-endeavor]

i think message can just be empty. we can attach error details

import (
    "context"

    "google.golang.org/grpc/codes"
    "google.golang.org/grpc/status"

    "google.golang.org/genproto/googleapis/rpc/errdetails"
)

func (s *Server) CreateUser(ctx context.Context, req *pb.CreateUserRequest) (*pb.CreateUserResponse, error) {
    var violations []*errdetails.BadRequest_FieldViolation

    if req.Email == "" {
        violations = append(violations, &errdetails.BadRequest_FieldViolation{
            Field:       "email",
            Description: "email is required",
        })
    }

    if req.Age < 18 {
        violations = append(violations, &errdetails.BadRequest_FieldViolation{
            Field:       "age",
            Description: "must be at least 18",
        })
    }

    if len(violations) > 0 {
        st := status.New(codes.InvalidArgument, "validation failed")

        br := &errdetails.BadRequest{
            FieldViolations: violations,
        }

        stWithDetails, err := st.WithDetails(br)
        if err != nil {
            return nil, st.Err() // fallback
        }

        return nil, stWithDetails.Err()
    }

    return &pb.CreateUserResponse{UserId: "123"}, nil
}

upon getting a validation error, the sdk/yaml client should call the get for edit endpoint again, and show the validation error message.

Maybe upon success we return the new version, but i think that is even not needed.