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.

Sign up for GitHub

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

Jump to bottom

feat: Support errors on NCP status #895

Merged
merged 1 commit into from
May 28, 2024
Merged

feat: Support errors on NCP status #895

merged 1 commit into from
May 28, 2024

Conversation

rollandf
Copy link
Member

Fixes #533

Example status:

status:
  appliedStates:
  - name: state-multus-cni
    state: ignore
  - name: state-container-networking-plugins
    state: ignore
  - name: state-ipoib-cni
    state: ignore
  - name: state-whereabouts-cni
    state: ignore
  - error: 'failed to create k8s objects from manifest: failed to render objects:
      ForcePrecompiled is enabled and precompiled image was not found'
    name: state-OFED
    state: notReady
  - name: state-SRIOV-device-plugin
    state: ignore

@rollandf rollandf added the on hold This enhancement is currently on hold pending additional clarification and evaluation label Apr 17, 2024
@rollandf
Copy link
Member Author

Need to add unit tests, and check how the different states are behaving

@killianmuldoon
Copy link
Contributor

Need to be a bit careful with this as any error which is different on each reconcile - e.g. which contains the generated name of an object - will cause infinite super-fast reconciles

@rollandf rollandf removed the on hold This enhancement is currently on hold pending additional clarification and evaluation label May 15, 2024
@rollandf
Copy link
Member Author

Need to be a bit careful with this as any error which is different on each reconcile - e.g. which contains the generated name of an object - will cause infinite super-fast reconciles

AFAIK, the generated yamls are the same on each reconcile. Otherwise, we would have tons of k8s objects created.

@rollandf
Copy link
Member Author

/retest-nic_operator_helm

@rollandf
Copy link
Member Author

/retest-nic_operator_kind

adrianchiris
adrianchiris reviewed May 23, 2024
View reviewed changes
api/v1alpha1/nicclusterpolicy_types.go Outdated
@@ -291,6 +291,8 @@ type AppliedState struct {
Name string `json:"name"`
// +kubebuilder:validation:Enum={"ready", "notReady", "ignore", "error"}
State State `json:"state"`
// Error string in case the state is error
Error string `json:"error,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we call this "message" ?

// message is a human readable message indicating details about why the state is in this condition.

similar to what we have in pod status

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

adrianchiris
adrianchiris reviewed May 23, 2024
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment •
edited

Choose a reason for hiding this comment

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

small comment about naming otherwise LGTM

@coveralls
Copy link
Collaborator

coveralls commented May 23, 2024 •
edited

Pull Request Test Coverage Report for Build 9250532710

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • 44 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.6%) to 63.392%
Files with Coverage Reduction New Missed Lines %
pkg/docadriverimages/doca_drivers.go 44 0.0%
Totals Coverage Status
Change from base Build 9242901315: -0.6%
Covered Lines: 3226
Relevant Lines: 5089
💛 - Coveralls

@rollandf
Copy link
Member Author

/retest-copy-right

@rollandf
Copy link
Member Author

/retest-image_scan

@rollandf
Copy link
Member Author

/retest-blackduck_scan

adrianchiris
adrianchiris previously approved these changes May 23, 2024
View reviewed changes
@rollandf
Copy link
Member Author

/retest-nic_operator_helm

@rollandf
Copy link
Member Author

/retest-nic_operator_kind

6 similar comments
@adrianchiris
Copy link
Collaborator

/retest-nic_operator_kind

@rollandf
Copy link
Member Author

/retest-nic_operator_kind

@rollandf
Copy link
Member Author

/retest-nic_operator_kind

@rollandf
Copy link
Member Author

/retest-nic_operator_kind

@rollandf
Copy link
Member Author

/retest-nic_operator_kind

@rollandf
Copy link
Member Author

/retest-nic_operator_kind

@rollandf
feat: Support errors on NCP status
f8bc1ab 
Fixes Mellanox#533

Signed-off-by: Fred Rolland <frolland@nvidia.com>
ykulazhenkov
ykulazhenkov approved these changes May 27, 2024
View reviewed changes
Copy link
Collaborator

@ykulazhenkov ykulazhenkov left a comment

Choose a reason for hiding this comment

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

LGTM

adrianchiris
adrianchiris approved these changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

LGTM!

@adrianchiris adrianchiris merged commit 09cdb17 into Mellanox:master May 28, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrianchiris adrianchiris adrianchiris approved these changes

@ykulazhenkov ykulazhenkov ykulazhenkov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Errors are not reported in CR status
5 participants
@rollandf @killianmuldoon @coveralls @adrianchiris @ykulazhenkov