-
Notifications
You must be signed in to change notification settings - Fork 258
Expanding mtpnc status to include delete status #4085
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Expands MultitenantPodNetworkConfig visibility and lifecycle status tracking.
- Adds PodUID and Status printer columns; removes PodNetwork printer column.
- Introduces new MTPNCStatusDeleting status constant.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
crd/multitenancy/manifests/multitenancy.acn.azure.com_multitenantpodnetworkconfigs.yaml | Adjusts CRD printer columns to include PodUID and Status, removes PodNetwork. |
crd/multitenancy/api/v1alpha1/multitenantpodnetworkconfig.go | Adds PodUID and Status printcolumn annotations and new Deleting status constant. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
MTPNCStatusPNINotReady MTPNCStatus = "PNINotReady" | ||
MTPNCStatusNodeCapacityExceeded MTPNCStatus = "NodeCapacityExceeded" | ||
MTPNCStatusIPsExhausted MTPNCStatus = "IPsExhausted" | ||
MTPNCStatusDeleting MTPNCStatus = "Deleting" |
Copilot
AI
Oct 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The new status MTPNCStatusDeleting is introduced without accompanying documentation on when it is set or how controllers should transition from/to it. Please add a brief comment above the status constants explaining each status, especially Deleting (e.g., whether it indicates a finalizer in progress, resource cleanup, or a terminal state).
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think copilot is on to something here.
adding a deleted status seems risky. what sets it? if i do a kubectl delete ...
i'm not setting that status. so there's an opportunity for anything which uses this status to learn that the mtpnc is deleting to miss that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be set by dnc-rc as the mtpnc finalizer - we can document the semantics here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but what if it doesn't? this can't be anything more than advisory, because if you depend on it you can miss it because it's not actually hardlinked to the object being deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not taking any programmatic dependency on it, we don't want mtpnc to have ready state while it's actually getting deleted.
Reason for Change:
Issue Fixed:
Requirements:
Notes: