Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor label parsing to set OF labels last #229

Conversation

LucasRoesler
Copy link
Member

Description

  • Ensure that internal OF labels are set after user labels to ensure
    that users do no override these internal values
  • Refactor the getMinReplicaCount to work with the labels pointer, this
    helps simplify the control flow in the handler methods
  • Add constants for the label keys and update all references to those
    keys throughout

Motivation and Context

  • I have raised an issue to propose this change (required)

Closes #209

How Has This Been Tested?

Unit tests have been added and I have tested in minikube

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@LucasRoesler
Copy link
Member Author

LucasRoesler commented Jun 23, 2018

Question for @alexellis and @stefanprodan, while writing this I noticed that we are using pointers to maps in the request. Concretely request.Labels is a *map[string]string, is there a specific reason for this? Maps are already nil-able without the pointer. I did a couple experiments in the go playground
and the concrete map will still be nil if the field is omitted in the request json. So, unlike with strings, it doesn't seem that we need the pointer here to make the field optional. It seems that we could just use a plain map[string]string instead of the pointer. I think this make some of the code a little more straightforward, since *map[string]string isn't indexable while map[string]string is.

Edit: here is the reference playground snippet https://play.golang.org/p/8h5ufOIjn5b

@alexellis
Copy link
Member

There will be a Go-specific reason for using a reference here. Wouldn't changing it be a breaking change to the whole project?

Example of indexing: https://play.golang.org/p/cFdZ-Agh6ii

// OFFunctionNameLabel is the label key used by OpenFaaS to store the function name
// on the resources managed by OpenFaaS for that function. This key is also used to
// denote that a resource is a "Function"
OFFunctionNameLabel = "faas_function"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the OF prefix add?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, those aren't needed. I will remove them


// getMinReplicaCount extracts the functions minimum replica count from the user's
// request labels. If the value is not found, this will return the default value, 1.
func getMinReplicaCount(labels *map[string]string) *int32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to return an error than logging an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update it to return an error, I was simply moved the existing function. The error printing was in the original function.

Copy link
Member Author

@LucasRoesler LucasRoesler Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected error behavior if we can't parse the replica count value? Or if the value is non-valid? should the caller (the handler in or a helper to the handler) also return the error or should it ignore the error and set it to the minimum, which is the current behavior?


// parseLabels will copy the user request labels and ensure that any required internal labels
// are set appropriately.
func parseLabels(functionName string, requestLables *map[string]string) map[string]string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing any parsing in parseLabels. Is this the correct name for the function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable alternatives could be buildLabels or getLabels

}

initialReplicas := getMinReplicaCount(request.Labels)
labels := parseLabels(request.Service, request.Labels)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the UID value should also be added here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I originally intended to fold that into the function but wasn't very sure if since the logic only existed in the update

@LucasRoesler
Copy link
Member Author

I have pushed changes for each of your comments, they are individual commits to make it easier to review each change. I can squash the commits once it is approved.

Thinking more about how changing the *map would impact the project, you are right that it would breaking change, since each of the OF projects imports requests and references that struct. In theory it should just be a dependency update in each of the projects, but it is probably not worth the headache.

I was mostly curious from an academic perspective if there was a specific feature or capability that using a pointer there enabled? It isn't obvious to me and I am just looking to learn more.

@LucasRoesler LucasRoesler force-pushed the feature-update-label-parsing-to-ensure-internal-labels-are-preserved branch from dca3875 to b24d6fe Compare June 23, 2018 18:03
@LucasRoesler
Copy link
Member Author

I just rebased on master and I am now getting build errors

test/read_config_test.go:111:12: config.EnableFunctionReadinessProbe undefined (type "github.com/openfaas/faas-netes/types".BootstrapConfig has no field or method EnableFunctionReadinessProbe)

@LucasRoesler LucasRoesler force-pushed the feature-update-label-parsing-to-ensure-internal-labels-are-preserved branch from b24d6fe to da51428 Compare August 3, 2018 08:44
**What**
- Ensure that internal OF labels are set after user labels to ensure
that users do no override these internal values
- Refactor the getMinReplicaCount to work with the labels pointer, this
helps simplify the control flow in the handler methods
- Add constants for the label keys and update all references to those
keys throughout

Closes openfaas#209

Signed-off-by: Lucas Roesler <[email protected]>
**What**
- moves the uid label into a constant
- sets it inside the labels method so that it is applied consistently

Signed-off-by: Lucas Roesler <[email protected]>
**What**
- rename parseLabels to buildLables to make the name more semantic

Signed-off-by: Lucas Roesler <[email protected]>
**What**
- Return parsing errors from getMinReplicaCount
- Add tests for the expected errors

Signed-off-by: Lucas Roesler <[email protected]>
@LucasRoesler LucasRoesler force-pushed the feature-update-label-parsing-to-ensure-internal-labels-are-preserved branch from da51428 to 6edf7e3 Compare October 7, 2018 07:43
@alexellis
Copy link
Member

I need more time to understand this before merging.

@LucasRoesler LucasRoesler deleted the feature-update-label-parsing-to-ensure-internal-labels-are-preserved branch September 4, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible override of internal labels by the request labels
2 participants