Refactor label parsing to set OF labels last #229

LucasRoesler · 2018-06-23T09:46:19Z

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 · 2018-06-23T10:44:10Z

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 · 2018-06-23T10:59:29Z

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

alexellis · 2018-06-23T11:00:26Z

handlers/labels.go

+	// 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"


What does the OF prefix add?

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

alexellis · 2018-06-23T11:01:48Z

handlers/labels.go

+
+// 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 {


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

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

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?

alexellis · 2018-06-23T11:03:17Z

handlers/labels.go

+
+// 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 {


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

Reasonable alternatives could be buildLabels or getLabels

alexellis · 2018-06-23T11:05:21Z

handlers/deploy.go

-	}
-
+	initialReplicas := getMinReplicaCount(request.Labels)
+	labels := parseLabels(request.Service, request.Labels)


I think the UID value should also be added here.

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 · 2018-06-23T16:54:56Z

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 · 2018-07-23T08:28:48Z

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)

**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]>

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]>

alexellis · 2019-07-05T08:11:27Z

I need more time to understand this before merging.

alexellis reviewed Jun 23, 2018

View reviewed changes

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 force-pushed the feature-update-label-parsing-to-ensure-internal-labels-are-preserved branch from b24d6fe to da51428 Compare August 3, 2018 08:44

LucasRoesler added 5 commits October 7, 2018 09:41

Update label constants to remove OF prefix

5344c15

Signed-off-by: Lucas Roesler <[email protected]>

Move the uid into teh common label method

8d22ce8

**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]>

Rename parseLabels to buildLabels

3413f25

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

Propagate getMinReplicaCount error to the handler

6edf7e3

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

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 added the blocked label Jul 5, 2019

LucasRoesler mentioned this pull request Aug 5, 2019

Question: Why are functions updated when they are deployed? #484

Closed

LucasRoesler closed this Oct 23, 2021

LucasRoesler deleted the feature-update-label-parsing-to-ensure-internal-labels-are-preserved branch September 4, 2023 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor label parsing to set OF labels last #229

Refactor label parsing to set OF labels last #229

LucasRoesler commented Jun 23, 2018

LucasRoesler commented Jun 23, 2018 •

edited

Loading

alexellis commented Jun 23, 2018

alexellis Jun 23, 2018

LucasRoesler Jun 23, 2018

alexellis Jun 23, 2018

LucasRoesler Jun 23, 2018

LucasRoesler Jun 23, 2018 •

edited

Loading

alexellis Jun 23, 2018

LucasRoesler Jun 23, 2018

alexellis Jun 23, 2018

LucasRoesler Jun 23, 2018

LucasRoesler commented Jun 23, 2018

LucasRoesler commented Jul 23, 2018

alexellis commented Jul 5, 2019

Refactor label parsing to set OF labels last #229

Refactor label parsing to set OF labels last #229

Conversation

LucasRoesler commented Jun 23, 2018

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

LucasRoesler commented Jun 23, 2018 • edited Loading

alexellis commented Jun 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasRoesler Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasRoesler commented Jun 23, 2018

LucasRoesler commented Jul 23, 2018

alexellis commented Jul 5, 2019

LucasRoesler commented Jun 23, 2018 •

edited

Loading

LucasRoesler Jun 23, 2018 •

edited

Loading