Conversation
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Summary of ChangesHello @SchSeba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the observability and operational transparency of the SR-IOV Network Operator by integrating standard Kubernetes conditions into the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust implementation of Kubernetes conditions for SriovNetworkNodeState, significantly improving its observability. The changes are well-structured, with new helper functions for managing conditions, comprehensive tests, and a detailed design document. The use of Patch for status updates and careful state management correctly handles potential race conditions between the drain controller and the config daemon. A notable improvement is the immediate reporting of drain errors, like PDB violations, by capturing log output from the drain helper, which enhances the responsiveness of the DrainDegraded condition.
| // ErrOut captures errors from the drain library, including retry errors. | ||
| // We call the onError callback here to report eviction failures immediately, | ||
| // before the drain timeout is reached. | ||
| ErrOut: writer{func(msg string, kv ...interface{}) { | ||
| cleanMsg := strings.ReplaceAll(msg, "\n", "") | ||
| logger.Error(nil, cleanMsg, kv...) | ||
| // Call the error callback for eviction/deletion errors | ||
| if onError != nil && strings.Contains(cleanMsg, "error when") { | ||
| onError(fmt.Errorf("%s", cleanMsg)) | ||
| } | ||
| }}, |
There was a problem hiding this comment.
This implementation for capturing drain errors by parsing log output is clever but brittle, as it depends on the specific log message format of the upstream k8s.io/kubectl/pkg/drain library. A change in the upstream logging could break this error detection.
It would be beneficial to add a comment here explaining this dependency and the trade-off made. This will help future maintainers understand why this approach was chosen (e.g., to get immediate feedback on retryable errors like PDB violations, which OnPodDeletionOrEvictionFinished doesn't provide) and to be aware of the potential for breakage if the upstream library is updated.
SchSeba
force-pushed
the
conditions/nodestate
branch
from
e4e96ca to
230d43c
Compare
Pull Request Test Coverage Report for Build 22143114570Details
💛 - Coveralls |
Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Add foundational condition support for SR-IOV Network Operator CRDs: - Define standard condition types (Ready, Progressing, Degraded) - Define drain-specific condition types (DrainProgressing, DrainDegraded, DrainComplete) - Add DrainState enum for tracking drain operation states - Add common condition reasons for various states - Add NetworkStatus type with Conditions field for network CRDs - Add ConditionsEqual helper for comparing conditions ignoring LastTransitionTime - Add SetConfigurationConditions and SetDrainConditions methods - Add comprehensive unit tests for condition handling This follows the Kubernetes API conventions for status conditions and provides the foundation for observability improvements across all SR-IOV CRDs. Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Add a new status package with utilities for managing CRD status updates: Patcher (patcher.go): - Provides retry logic for status updates with conflict handling - Supports both Update and Patch operations - Includes UpdateStatusWithEvents for automatic event emission - Embeds condition management methods for convenience - Uses interface for easy mocking in tests Transitions (transitions.go): - DetectTransitions compares old and new conditions - Returns structured Transition objects for each change - EventType() method returns appropriate event type (Normal/Warning) - EventReason() generates suitable event reason strings - Supports Added, Changed, Removed, and Unchanged transitions Tests: - Comprehensive unit tests for patcher operations - Tests for transition detection logic - Tests for event type and reason generation This package provides a reusable foundation for consistent status handling across all controllers in the operator. Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Update SriovNetworkNodeState status type to include Kubernetes conditions: - Add Conditions field to status - Add printcolumns for Ready, Progressing, Degraded - Add printcolumns for drain conditions (DrainProgress, DrainDegraded, DrainComplete) Conditions follow Kubernetes conventions with patchMergeKey and listType annotations for proper strategic merge patch support. Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Update config daemon status handling to set configuration conditions: - Call SetConfigurationConditions when updating node state sync status - Preserve drain conditions during configuration status updates - Use ConditionsEqual to avoid unnecessary API calls when unchanged - Set Ready=True, Progressing=False, Degraded=False on success - Set Progressing=True, Ready=False on in-progress configuration - Set Degraded=True with error message on configuration failure - Keep Degraded=True while retrying after previous failure This ensures the daemon properly reports configuration progress and errors through standard Kubernetes conditions, enabling better observability and automation. Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Add comprehensive drain condition management to the drain controller: Drain Controller Changes: - Add updateDrainConditions helper to set drain-specific conditions - Set DrainProgressing=True when drain starts - Set DrainDegraded=True when drain errors occur (e.g., PDB violations) - Set DrainComplete=True when drain completes successfully - Reset all drain conditions to idle state when returning to idle - Use Patch instead of Update to avoid race conditions with daemon Drainer Changes: - Add DrainErrorCallback type for immediate error notification - Call callback when eviction errors occur (before timeout) - Capture first real error (e.g., PDB violation) instead of generic timeout - Pass callback through cordon and drain operations Controller Helper Fix: - Fix DrainStateAnnotationPredicate to use GetAnnotations() not GetLabels() Tests: - Add tests for DrainComplete=True after successful drain - Add tests for DrainDegraded=False after successful drain - Add tests for idle state conditions - Add tests for observedGeneration consistency - Add tests for single node reboot drain conditions - Add tests for error callback invocation This enables users to observe drain progress and errors through standard Kubernetes conditions, improving troubleshooting and enabling automated responses to drain failures. Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Update the kubernetes-conditions-integration.md design document to reflect the actual implementation details: - Add SriovNetworkNodePolicy as a supported CRD - Add drain-specific conditions (DrainProgressing, DrainDegraded, DrainComplete) - Document all condition reasons with actual constant names - Add NetworkStatus shared type used by network CRDs - Document pkg/status package with Patcher interface and transition detection - Add ConditionsEqual helper function documentation - Update API extension examples with actual struct definitions - Add matchedNodeCount/readyNodeCount to Policy and PoolConfig status - Add kubectl output examples showing new columns - Update implementation commits list - Mark document status as 'implemented' Signed-off-by: Sebastian Sch <sebassch@gmail.com>
- Add SetConfigurationConditions and SetDrainConditions methods to helper.go - Add findCondition helper function to suite_test.go - Regenerate CRDs and deepcopy for SriovNetworkNodeState type Signed-off-by: Sebastian Sch <sebassch@gmail.com>
The optimization to skip unnecessary API calls in updateSyncState was only checking SyncStatus, LastSyncError, and Conditions. This caused updates to Status.Interfaces, Status.Bridges, and Status.System to be skipped when those fields changed but the other fields remained the same. This broke ExternallyManaged policy validation because when VFs were manually configured on the host, the daemon would discover the new NumVfs value but skip updating the SriovNetworkNodeState status. The webhook would then see stale NumVfs=0 and reject the policy. Add equality checks for Interfaces, Bridges, and System fields to ensure host status changes are properly persisted to the API. Signed-off-by: Cursor AI <noreply@cursor.com>
SchSeba
force-pushed
the
conditions/nodestate
branch
from
230d43c to
de1e8d4
Compare
- sriovoperatorconfig_controller_test: add explicit timeout to the Eventually call that waits for the controller to add finalizers. The default 1s timeout is too tight for CI environments where the controller manager may take longer to start reconciling. - daemon_test: trigger an annotation change after creating a fresh nodeState in BeforeEach to guard against a race where the informer cache misses the initial create event with controller-runtime's priority queue, causing the daemon to never reconcile the object. Signed-off-by: Sebastian Sch <sebassch@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
No description provided.