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

capture: Calculated captured state from nmstatectl #998

Merged

Conversation

qinqon
Copy link
Member

@qinqon qinqon commented Feb 24, 2022

Is this a BUG FIX or a FEATURE ?:

Uncomment only one, leave it on its own line:

/kind bug

/kind enhancement

What this PR does / why we need it:
Calculating the captured state with the NNS is problematic since there
are some latencies between nmstatectl set and NNS being updated, so it
could happend that next NNCP is calculated with inaccurate network
configuration. This change read the currentState from nmstatectl to get
accurate data.

Special notes for your reviewer:
Fix bz https://bugzilla.redhat.com/show_bug.cgi?id=2057613

Release note:

Calculate captured state from nmstatectl show outpout

@kubevirt-bot kubevirt-bot added kind/bug release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 24, 2022
@qinqon qinqon force-pushed the use-nmstatectl-to-calculate-captured-state branch from 6ddb538 to a471fd3 Compare February 24, 2022 13:10
Copy link
Collaborator

@rhrazdil rhrazdil left a comment

Choose a reason for hiding this comment

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

The fix looks good.
@qinqon Interesting that we didn't hit this issue in u/s CI, do you think it's worth adding a test scenario that reproduced this in a followup?

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2022
@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhrazdil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2022
@qinqon
Copy link
Member Author

qinqon commented Feb 24, 2022

/cherry-pick release-0.64

@kubevirt-bot
Copy link
Collaborator

@qinqon: once the present PR merges, I will cherry-pick it on top of release-0.64 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.64

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Calculating the captured state with the NNS is problematic since there
are some latencies between nmstatectl set and NNS being updated, so it
could happend that next NNCP is calculated with inaccurate network
configuration. This change read the currentState from nmstatectl to get
accurate data.

Signed-off-by: Quique Llorente <ellorent@redhat.com>
@qinqon qinqon force-pushed the use-nmstatectl-to-calculate-captured-state branch from a471fd3 to ef4c068 Compare February 24, 2022 14:28
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2022
@rhrazdil
Copy link
Collaborator

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2022
@qinqon
Copy link
Member Author

qinqon commented Feb 24, 2022

/retest

@qinqon
Copy link
Member Author

qinqon commented Feb 25, 2022

The fix looks good. @qinqon Interesting that we didn't hit this issue in u/s CI, do you think it's worth adding a test scenario that reproduced this in a followup?

Since it's a race it depends how much time happends between one NNCP and the next, also what do we do on the next NNCP, if both NNCP use capture and one capture is using the previous as input then we can end with the issue at hand.

@qinqon
Copy link
Member Author

qinqon commented Feb 28, 2022

/retest

@rhrazdil
Copy link
Collaborator

Since it's a race it depends how much time happends between one NNCP and the next, also what do we do on the next NNCP, if both NNCP use capture and one capture is using the previous as input then we can end with the issue at hand.

Thanks, understood.

@qinqon
Copy link
Member Author

qinqon commented Feb 28, 2022

/cherry-pick release-0.64

@kubevirt-bot
Copy link
Collaborator

@qinqon: once the present PR merges, I will cherry-pick it on top of release-0.64 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.64

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@qinqon
Copy link
Member Author

qinqon commented Feb 28, 2022

/retest

It suffers from DNS resolve ping, let's retry

: failed checking DNS connectivity: [failed looking up NS root-server.net using name sever 192.168.66.2: lookup root-server.net on [fd00::1]:53: server misbehaving failed looking up NS root-server.net using name sever fd00::1: lookup root-server.net on [fd00::1]:53: dial udp [fd00::1]:53: connect: network is unreachable]: failed running probes after rollback: failed runnig probe 'dns' with after network reconfiguration -> currentState: ---

@qinqon
Copy link
Member Author

qinqon commented Feb 28, 2022

/retest

@rhrazdil
Copy link
Collaborator

This time, the go build failed again
/test pull-kubernetes-nmstate-e2e-handler-k8s

@qinqon
Copy link
Member Author

qinqon commented Feb 28, 2022

This time, the go build failed again /test pull-kubernetes-nmstate-e2e-handler-k8s

ARM build sometimes fail...

@kubevirt-bot kubevirt-bot merged commit 44670de into nmstate:main Feb 28, 2022
@kubevirt-bot
Copy link
Collaborator

@qinqon: #998 failed to apply on top of branch "release-0.64":

Applying: capture: Calculated captured state from nmstatectl
Using index info to reconstruct a base tree...
M	controllers/handler/nodenetworkconfigurationpolicy_controller.go
Falling back to patching base and 3-way merge...
Auto-merging controllers/handler/nodenetworkconfigurationpolicy_controller.go
CONFLICT (content): Merge conflict in controllers/handler/nodenetworkconfigurationpolicy_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 capture: Calculated captured state from nmstatectl
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.64

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

qinqon added a commit to qinqon/kubernetes-nmstate that referenced this pull request Mar 1, 2022
Calculating the captured state with the NNS is problematic since there
are some latencies between nmstatectl set and NNS being updated, so it
could happend that next NNCP is calculated with inaccurate network
configuration. This change read the currentState from nmstatectl to get
accurate data.

Cherry picked from (nmstate#998)

Signed-off-by: Quique Llorente <ellorent@redhat.com>
kubevirt-bot pushed a commit that referenced this pull request Mar 1, 2022
Calculating the captured state with the NNS is problematic since there
are some latencies between nmstatectl set and NNS being updated, so it
could happend that next NNCP is calculated with inaccurate network
configuration. This change read the currentState from nmstatectl to get
accurate data.

Cherry picked from (#998)

Signed-off-by: Quique Llorente <ellorent@redhat.com>
creydr pushed a commit to creydr/kubernetes-nmstate that referenced this pull request Apr 27, 2022
Calculating the captured state with the NNS is problematic since there
are some latencies between nmstatectl set and NNS being updated, so it
could happend that next NNCP is calculated with inaccurate network
configuration. This change read the currentState from nmstatectl to get
accurate data.

Signed-off-by: Quique Llorente <ellorent@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants