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

nodeAffinity is not correctly updated with DeepDerivative #47

Merged

Conversation

zshi-redhat
Copy link
Collaborator

@zshi-redhat zshi-redhat requested a review from fedepaol January 18, 2021 10:32
@fedepaol
Copy link
Collaborator

Ouch! Couple of questions:

  • Is this because the nodeAffinity is reset when deleting the policy (and as a 0 field is not compared)?
  • Should we replace the other occurrences of DeepDerivative too?
  • Did you check if this brings back the unwanted extra reconciliation loops?

if equality.Semantic.DeepDerivative(in.Spec, ds.Spec) {

// Note: with DeepDerivative, controller has issue not updating nodeAffinity correctly:
// https://bugzilla.redhat.com/show_bug.cgi?id=1914066, Chnange it to DeepEqual()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: "chnange".
I also wonder if we should delete the comment abovee

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can delete previous comment if you prefer, just thought it might be useful for reference if later we hit the endless loop issue again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a strong opinion, I guess it's fine since the comment below explains it pretty clearly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed typo

@zshi-redhat
Copy link
Collaborator Author

Ouch! Couple of questions:

  • Is this because the nodeAffinity is reset when deleting the policy (and as a 0 field is not compared)?

NodeAffinity is not updated when deleting policy, the legacy nodeAffinity remains in the daemonset spec.

  • Should we replace the other occurrences of DeepDerivative too?

I didn't know any issue with the other one, I think we can keep it as it is for now.

  • Did you check if this brings back the unwanted extra reconciliation loops?

Yes, there is no endless loop in sriov operator log with DeepEqual.

@fedepaol
Copy link
Collaborator

LGTM (apart from the typo)

@zshi-redhat zshi-redhat force-pushed the sriov-plugin-node-affinity branch from ae33d79 to 6881552 Compare January 18, 2021 14:04
@fedepaol
Copy link
Collaborator

LGTM

@pliurh
Copy link
Collaborator

pliurh commented Jan 19, 2021

With DeepEqual, I suppose we will hit the issue which @fedepaol tried to fix since it will be always false. Can we add a condition to check whether the nodeAffinity should be removed from the spec of DS, instead of using DeepEqual?

@zshi-redhat zshi-redhat force-pushed the sriov-plugin-node-affinity branch from 6881552 to 4b0bafa Compare January 21, 2021 12:22
@zshi-redhat
Copy link
Collaborator Author

With DeepEqual, I suppose we will hit the issue which @fedepaol tried to fix since it will be always false. Can we add a condition to check whether the nodeAffinity should be removed from the spec of DS, instead of using DeepEqual?

Done

@pliurh pliurh merged commit 4beb329 into k8snetworkplumbingwg:master Jan 22, 2021
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.

3 participants