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

Use downward API to pass current spec.nodeName to pod #309

Merged

Conversation

andreaskaris
Copy link
Contributor

The podInformerFactory uses filter key spec.nodeName to filter the pods that it should monitor. Up until now, this filter was set to the value of HOSTNAME. However, this is not reliable, as spec.nodeName can be overridden in kubernetes with --hostname-override and thus HOSTNAME and spec.nodeName do now necessarily always match. Instead, rely on a new custom environment variable NODENAME which is populated by the downward API.

Fixes https://issues.redhat.com/browse/OCPBUGS-10364

@andreaskaris andreaskaris marked this pull request as draft March 15, 2023 19:16
@coveralls
Copy link

coveralls commented Mar 15, 2023

Pull Request Test Coverage Report for Build 4436337857

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 30 of 30 (100.0%) changed or added relevant lines in 3 files are covered.
  • 58 unchanged lines in 3 files lost coverage.
  • Overall coverage remained the same at 71.009%

Files with Coverage Reduction New Missed Lines %
pkg/controlloop/entity_generators.go 2 97.85%
pkg/controlloop/dummy_controller.go 16 77.88%
pkg/controlloop/pod.go 40 83.4%
Totals Coverage Status
Change from base Build 4428384184: 0.0%
Covered Lines: 943
Relevant Lines: 1328

💛 - Coveralls

@andreaskaris andreaskaris force-pushed the nodename-from-downward-api branch 2 times, most recently from afe0d54 to 059409b Compare March 16, 2023 11:06
@andreaskaris andreaskaris changed the title [wip] Use downward API to pass current spec.nodeName to pod Use downward API to pass current spec.nodeName to pod Mar 16, 2023
@andreaskaris andreaskaris marked this pull request as ready for review March 16, 2023 11:21
Copy link
Collaborator

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

Conceptually, this makes total sense - this had slipped me by - full FQDNs will not work.

Thanks for catching.

Could you return an error instead, and throw in a couple of unit tests for this ?
... sorry for the lack of current tests in this package ...

cmd/controlloop/controlloop.go Outdated Show resolved Hide resolved
@maiqueb
Copy link
Collaborator

maiqueb commented Mar 16, 2023

@andreaskaris for the unit tests, you'll have to change the signature of

func newPodController(stopChannel chan struct{}) (*controlloop.PodController, error) {
allowing you to inject the client set into it.

K8S has a mock client set where you can provision it with the objects it would be able to retrieve: that way, you can write a negative and a positive test.

EDIT: something like what we're doing here:

k8sClient = fakek8sclient.NewSimpleClientset(pod)

@andreaskaris andreaskaris force-pushed the nodename-from-downward-api branch 2 times, most recently from 091f13b to 888e459 Compare March 16, 2023 17:47
The podInformerFactory uses filter key spec.nodeName to filter the pods
that it should monitor. Up until now, this filter was set to the value
of HOSTNAME. However, this is not reliable, as spec.nodeName can be
overridden in kubernetes with --hostname-override and thus HOSTNAME and
spec.nodeName do not necessarily always match. Instead, rely on a new
custom environment variable NODENAME which is populated by the downward
API.

Signed-off-by: Andreas Karis <ak.karis@gmail.com>
@andreaskaris andreaskaris force-pushed the nodename-from-downward-api branch from 888e459 to ef409bb Compare March 16, 2023 17:52
@andreaskaris
Copy link
Contributor Author

I addressed the unit tests a bit differently via the newDummyPodController @maiqueb @dougbtv mainly because I had already started rewriting it before I read the comments here. Let me know what you think - if that's not good enough, I can dig into @maiqueb 's suggestion

@andreaskaris andreaskaris requested review from maiqueb and removed request for dougbtv March 16, 2023 18:29
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

I'm good with the approach on the unit tests

@dougbtv dougbtv merged commit d2dbf8a into k8snetworkplumbingwg:master Mar 21, 2023
mgfritch added a commit to mgfritch/helm-charts that referenced this pull request Aug 11, 2023

Verified

This commit was signed with the committer’s verified signature.
mgfritch Michael Fritch
Pass `spec.nodeName` via the new `NODENAME` enviorment variable.

Introduced in whereabouts v0.6.2:
k8snetworkplumbingwg/whereabouts#309

Signed-off-by: Michael Fritch <mfritch@suse.com>
mgfritch added a commit to mgfritch/helm-charts that referenced this pull request Aug 11, 2023

Verified

This commit was signed with the committer’s verified signature.
mgfritch Michael Fritch
Update DaemonSet to pass `spec.nodeName` via a new `NODENAME`
environment variable.

Introduced by whereabouts v0.6.2:
k8snetworkplumbingwg/whereabouts#309

Signed-off-by: Michael Fritch <mfritch@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants