pkg/leader: allow setting the pod name explicitly. #617

squeed · 2018-10-15T16:09:23Z

This is useful for pods running in hostNetwork, where the hostname is not the pod name.

squeed · 2018-10-15T16:09:39Z

@mhrivnak PTAL, thanks!

smarterclayton · 2018-10-16T14:50:19Z

Assuming the hostname is super dangerous. I'm not sure this should ever be allowed. There can be two pods on the same node at the same time for any operator, and it's very easy to turn on host network without realizing it. We should be encouraging people to use the downward API from within their deployment to pass pod name as an env var / config flag all the time to prevent them accidentally having two leaders running.

smarterclayton · 2018-10-16T14:53:24Z

I would recommend changing the SDK to require a unique name and to instruct the user how to do it safely on Kubernetes. Selecting a random name is not safe either.

mhrivnak · 2018-10-16T15:15:43Z

@smarterclayton thanks! Sounds like we should just require the environment variable and never use the pod's hostname for pod name detection.

@squeed want to make that change in this PR, and have it stop using the hostname at all? It would also help to add a quick blurb at the end of the docs in doc.go that using the downward API for this is a requirement.

I'll be adding dev docs for this in the near future anyway and turning it on by default for new operators, so I'm happy to handle adding those docs at that time.

squeed · 2018-10-17T18:22:36Z

@mhrivnak PR updated, PTAL.

squeed · 2018-10-17T20:59:34Z

Test failure looks like a flake.

hasbro17 · 2018-10-17T21:33:02Z

@squeed Yeah seemed like an issue with the test cluster. I've reset the job and it passed.
LGTM

defer to @mhrivnak

Also we should add the POD_NAME env to the generated Deployment manifest so it's set by default via the downward API.
In the template:
https://github.com/operator-framework/operator-sdk/blob/master/pkg/scaffold/operator.go#L59
And the unit test:
https://github.com/operator-framework/operator-sdk/blob/master/pkg/scaffold/operator_test.go#L61

But maybe @mhrivnak can add that in when we turn on leader election by default.

hasbro17 · 2018-10-17T21:34:09Z

pkg/leader/leader.go

-		return nil, err
+	podName := os.Getenv(PodNameEnv)
+	if podName == "" {
+		return nil, errors.New(fmt.Sprintf("required env %s not set, please configure downward API", PodNameEnv))


Also errors.New(fmt.Sprintf ==> fmt.Errorf(

squeed · 2018-10-17T22:45:42Z

@hasbro17 good point; updated.

mhrivnak

Thanks! This looks great. If you'll just fix the tab vs space issue in the scaffolds, this will be ready to merge.

mhrivnak · 2018-10-16T15:17:17Z

pkg/leader/leader.go

@@ -154,14 +156,31 @@ func myNS() (string, error) {
 	return ns, nil
 }

+// myName returns the name of the pod in which this code is currently running


s/myName/myPodName/

mhrivnak · 2018-10-18T01:26:52Z

pkg/scaffold/operator.go

@@ -61,6 +61,10 @@ spec:
              valueFrom:
                fieldRef:
                  fieldPath: metadata.namespace
+			- name: POD_NAME


Could you just fix the indentation here? It appears there are tabs here vs spaces everywhere else, which explains the strange way it looks.

mhrivnak · 2018-10-18T01:27:00Z

pkg/scaffold/operator_test.go

@@ -63,6 +63,10 @@ spec:
              valueFrom:
                fieldRef:
                  fieldPath: metadata.namespace
+			- name: POD_NAME


squeed · 2018-10-22T13:39:01Z

Fixed the spacing issues (and rebased), PTAL.

lilic · 2018-10-22T16:10:57Z

/ok-to-test

mhrivnak · 2018-10-22T18:52:04Z

pkg/leader/doc.go

+  - name: POD_NAME
+	valueFrom:
+	  fieldRef:
+		fieldPath: metadata.name


Sorry, looks like there is one more case here of rogue tabs. Mind fixing that up?

We can't trust that the hostname is always the pod name.

knobunc · 2018-10-23T12:33:07Z

@mhrivnak do you have any idea why this PR is not merging?

mhrivnak · 2018-10-23T13:05:34Z

@mhrivnak do you have any idea why this PR is not merging?

Last I looked, I was just waiting to see the CI pass. Looks like that happened, so here we go!

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 15, 2018

squeed force-pushed the pod-name-env branch from 5800740 to 8ad38f3 Compare October 17, 2018 18:22

squeed force-pushed the pod-name-env branch from 8ad38f3 to 48f8f50 Compare October 17, 2018 18:24

hasbro17 reviewed Oct 17, 2018

View reviewed changes

squeed force-pushed the pod-name-env branch from 48f8f50 to 4f840c4 Compare October 17, 2018 22:45

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 17, 2018

mhrivnak suggested changes Oct 18, 2018

View reviewed changes

squeed force-pushed the pod-name-env branch from 4f840c4 to 1e958e9 Compare October 22, 2018 14:37

mhrivnak reviewed Oct 22, 2018

View reviewed changes

openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 22, 2018

pkg/leader: require the podname to be set explicitly

2c3795e

We can't trust that the hostname is always the pod name.

squeed force-pushed the pod-name-env branch from 1e958e9 to 2c3795e Compare October 22, 2018 20:15

mhrivnak approved these changes Oct 22, 2018

View reviewed changes

mhrivnak merged commit bc48c5c into operator-framework:master Oct 23, 2018

mhrivnak mentioned this pull request Jul 22, 2020

allow leader election to accept namespace operator-framework/operator-lib#1

Closed

pkg/leader: allow setting the pod name explicitly. #617

pkg/leader: allow setting the pod name explicitly. #617

Uh oh!

Conversation

squeed commented Oct 15, 2018

Uh oh!

squeed commented Oct 15, 2018

Uh oh!

smarterclayton commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Oct 16, 2018

Uh oh!

mhrivnak commented Oct 16, 2018

Uh oh!

squeed commented Oct 17, 2018

Uh oh!

squeed commented Oct 17, 2018

Uh oh!

hasbro17 commented Oct 17, 2018

Uh oh!

hasbro17 Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

squeed commented Oct 17, 2018

Uh oh!

mhrivnak left a comment

Choose a reason for hiding this comment

Uh oh!

mhrivnak Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

mhrivnak Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

mhrivnak Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

squeed commented Oct 22, 2018

Uh oh!

lilic commented Oct 22, 2018

Uh oh!

mhrivnak Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

knobunc commented Oct 23, 2018

Uh oh!

mhrivnak commented Oct 23, 2018

Uh oh!

Uh oh!

smarterclayton commented Oct 16, 2018 •

edited

Loading