Skip to content

tests, net, libs, nncp: On update/patch, assure correct status update - #733

Merged
myakove merged 4 commits into
RedHatQE:mainfrom
EdDev:wait-for-nncp-status-update
Apr 17, 2025
Merged

myakove merged 4 commits into
RedHatQE:mainfrom
EdDev:wait-for-nncp-status-update

Conversation

@EdDev

@EdDev EdDev commented Apr 10, 2025 •

Copy link
Copy Markdown
Contributor
What this PR does / why we need it:

When mutating an existing NNCP object, it is common to wait for a
condition status of success (condition type "Available").
However, it has been observed or bare-metal clusters that the observed
condition is the previous one and is not reflecting the actual state
after the spec update/patch.

In order to handle these cases, the condition timestamp is used to
assure it has been updated post the change.

Other alternatives have been found unsafe, like the option of waiting
for an intermediate state (e.g. "progressing"). It is unsafe because the
state is in transition and that can be quicker than the sampling
performed by the test tools.

The current NNCP object updates the spec only when performing the
clean-up step (on teardown). At that step the previous created
interfaces are being removed.
Any future exposure of update/patch requires this technique to assure
successful operation.


This change also introduces mypy validation on the new tests/network/libs package.

Which issue(s) this PR fixes:
Special notes for reviewer:

This is following the solution introduced in RedHatQE/openshift-python-wrapper#2354 with some tweaks.

jira-ticket:

Signed-off-by: Edward Haas <edwardh@redhat.com>
@ghost

ghost commented Apr 10, 2025

Copy link
Copy Markdown

Report bugs in Issues

The following are automatically added:

  • Add reviewers from OWNER file (in the root of the repository) under reviewers section.
  • Set PR size label.
  • New issue is created for the PR. (Closed when PR is merged/closed)
  • Run pre-commit if .pre-commit-config.yaml exists in the repo.

Available user actions:

  • To mark PR as WIP comment /wip to the PR, To remove it from the PR comment /wip cancel to the PR.
  • To block merging of PR comment /hold, To un-block merging of PR comment /hold cancel.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To cherry pick a merged PR comment /cherry-pick <target branch to cherry-pick to> in the PR.
    • Multiple target branches can be cherry-picked, separated by spaces. (/cherry-pick branch1 branch2)
    • Cherry-pick will be started when PR is merged
  • To build and push container image command /build-and-push-container in the PR (tag will be the PR number).
    • You can add extra args to the Podman build command
      • Example: /build-and-push-container --build-arg OPENSHIFT_PYTHON_WRAPPER_COMMIT=<commit_hash>
  • To add a label by comment use /<label name>, to remove, use /<label name> cancel
  • To assign reviewers based on OWNERS file use /assign-reviewers
  • To check if PR can be merged use /check-can-merge
  • to assign reviewer to PR use /assign-reviewer @<reviewer>
Supported /retest check runs
  • /retest tox: Retest tox
  • /retest build-container: Retest build-container
  • /retest all: Retest all
Supported labels
  • hold
  • verified
  • wip
  • lgtm

@EdDev

EdDev commented Apr 10, 2025

Copy link
Copy Markdown
Contributor Author

@rnetser , this is the follow up requested.
I tried to explain in the commit description the reasoning for this approach.

@yossisegev , @Anatw , I will need help to verify this one.

Signed-off-by: Edward Haas <edwardh@redhat.com>
@EdDev

EdDev commented Apr 14, 2025

Copy link
Copy Markdown
Contributor Author

change: Added new commit to remove condition class and fix cleanup method signature.

@Anatw Anatw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you, nice and elegant.

@EdDev

EdDev commented Apr 16, 2025

Copy link
Copy Markdown
Contributor Author

Verification:

  • Original issue recreated on a BM cluster without this change on the test that uses this new NNCP implementation.

Logs show:

2025-04-16T17:19:26.448770 ocp_resources NodeNetworkConfigurationPolicy INFO Update NodeNetworkConfigurationPolicy test-network-operator:
{'spec': {'desiredState': {'interfaces': [{'name': 'net-add-br', 'type': 'linux-bridge', 'state': 'absent', 'ipv4': {'enabled': False}, 'ipv6': {'enabled': False}, 'bridge': {'options': {'stp': {'enabled': False}}}}]}}, 'metadata': {'name': 'test-network-operator'}}
2025-04-16T17:19:26.467037 timeout_sampler INFO Waiting for 90 seconds [0:01:30], retry every 5 seconds. (Function: tests.network.libs.nodenetworkconfigurationpolicy.wait_for_status_success Args: (<tests.network.libs.nodenetworkconfigurationpolicy.NodeNetworkConfigurationPolicy object at 0x7f2fb38f2630>,))
2025-04-16T17:19:26.472459 ocp_resources NodeNetworkConfigurationPolicy INFO NodeNetworkConfigurationPolicy/test-network-operator configured successfully
2025-04-16T17:19:26.472715 timeout_sampler INFO Elapsed time: 0.0003421306610107422 [0:00:00.000342]

And checking that the bridge is still on the node confirmed the problem.

  • Applying this change proposed in this PR resolved the issue.

Logs show:

2025-04-16T17:27:57.807677 ocp_resources NodeNetworkConfigurationPolicy INFO Update NodeNetworkConfigurationPolicy test-network-operator:
{'spec': {'desiredState': {'interfaces': [{'name': 'net-add-br', 'type': 'linux-bridge', 'state': 'absent', 'ipv4': {'enabled': False}, 'ipv6': {'enabled': False}, 'bridge': {'options': {'stp': {'enabled': False}}}}]}}, 'metadata': {'name': 'test-network-operator'}}
2025-04-16T17:27:57.826354 timeout_sampler INFO Waiting for 90 seconds [0:01:30], retry every 5 seconds. (Function: tests.network.libs.nodenetworkconfigurationpolicy.wait_for_status_success Args: (<tests.network.libs.nodenetworkconfigurationpolicy.NodeNetworkConfigurationPolicy object at 0x7f0ca1b9fce0>,) Kwargs: {'last_success_timestamp': '2025-04-16T14:27:03Z'})
2025-04-16T17:28:02.842861 ocp_resources NodeNetworkConfigurationPolicy INFO NodeNetworkConfigurationPolicy/test-network-operator configured successfully
2025-04-16T17:28:02.843332 timeout_sampler INFO Elapsed time: 5.008163928985596 [0:00:05.008164]

And checking the node confirmed that the bridge is not there.

/verified

@myakove

myakove commented Apr 16, 2025 •

Copy link
Copy Markdown
Collaborator

/ldtm

@sbahar619 sbahar619 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm.

@myakove

myakove commented Apr 17, 2025

Copy link
Copy Markdown
Collaborator

/lgtm

@dbasunag1

Copy link
Copy Markdown

New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants