fix: Adding delete timestamp check #4078

shreyashastantram · 2025-10-14T23:45:59Z

Reason for Change:

Adding check to MTPNC to ensure that CNS doesn't respond back with the ipaddress if mtpnc is set for deletion.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

ashvindeodhar · 2025-10-15T21:27:08Z

cns/middlewares/k8sSwiftV2.go

 	if err := k.Cli.Get(ctx, mtpncNamespacedName, &mtpnc); err != nil {
 		return v1alpha1.MultitenantPodNetworkConfig{}, types.UnexpectedError, errors.Wrap(err, errGetMTPNC.Error()).Error()
 	}
+	if mtpnc.DeletionTimestamp != nil {


!IsZero() seems to be the way to check this. DeletionTimestamp is Time.
it may be helpful to add a func something like IsTerminating similar to IsReady that we already have on mtpnc.

I also feel we shouldn't be adding these checks inside getMTPNC func. This func should return the MTPNC object and the caller should look at it and decide how to proceed however it wants. (different callers may call getMTPNC and use different fields to decide what to do next)

I can add a function similar to isready, but we cannot use isZero since the data will be nil if it's not populated with timestamp. it's either nil or timestamp.

if the time.Time is absent, it will default to zero and IsZero() will be true

+1 @ashvindeodhar is right, this doesn't belong in the getMTPNC. Return the MTPNC and let the caller decide what to do if it's Deleting.

.DeletionTimestamp.IsZero() is canonically the correct way to determine if an Object is deleting. ref https://book.kubebuilder.io/reference/using-finalizers.html

nit: I would recommend a rename of isTerminating -> isDeleting everywhere (++ errMessage)
isDeleting ⇒ metadata.deletionTimestamp != nil
isTerminating ⇒ your CR’s .status.phase == "Terminating" (or whatever phase name your API defines) which doesn't apply to MTPNC

I do not see deleting being used anywhere in Kubernetes, keeping it, terminating will keep it consistent with the pod state terminating.

The pod status changes to Terminating until it’s fully removed,

Sends SIGTERM to containers.

Waits for the termination grace period (default 30s).

Runs any presto hooks.

Finally sends SIGKILL if containers don’t exit in time.

Unlike pod (where there is an actual termination phase), MTPNC doesn't have a termination phase. So, there is no need for us to keep it consistent with pod status. We should keep the name to the face value of the operation, since the errMessage is something, customers can see.

rbtr · 2025-10-15T22:06:30Z

cns/middlewares/k8sSwiftV2.go

 	errGetMTPNC                 = errors.New(NetworkNotReadyErrorMsg + " - failed to get MTPNC")
 	errInvalidSWIFTv2NICType    = errors.New("invalid NIC type for SWIFT v2 scenario")
 	errInvalidMTPNCPrefixLength = errors.New("invalid prefix length for MTPNC primaryIP, must be 32")
+	errDeleteTimestampSet       = errors.New("MTPNC deletion timestamp is set")


deletion timestamp being set is an implementation detail. the important thing is that the object is deleting.

will change it to mtpnc terminating

rbtr · 2025-10-15T22:09:38Z

cns/middlewares/k8sSwiftV2.go

 	if err := k.Cli.Get(ctx, mtpncNamespacedName, &mtpnc); err != nil {
 		return v1alpha1.MultitenantPodNetworkConfig{}, types.UnexpectedError, errors.Wrap(err, errGetMTPNC.Error()).Error()
 	}
+	if mtpnc.DeletionTimestamp != nil {


+1 @ashvindeodhar is right, this doesn't belong in the getMTPNC. Return the MTPNC and let the caller decide what to do if it's Deleting.

.DeletionTimestamp.IsZero() is canonically the correct way to determine if an Object is deleting. ref https://book.kubebuilder.io/reference/using-finalizers.html

Copilot

Pull Request Overview

This PR adds validation to prevent IP configuration requests for pods with terminating MultitenantPodNetworkConfig (MTPNC) resources by checking if the deletion timestamp is set.

Introduces an IsTerminating() method to check if a MTPNC resource has a deletion timestamp
Adds validation in the middleware to reject requests when MTPNC is terminating
Includes test coverage for the terminating state scenario

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
crd/multitenancy/api/v1alpha1/utils.go	Adds `IsTerminating()` method to check deletion timestamp
cns/middlewares/k8sSwiftV2.go	Implements validation to reject IP config requests when MTPNC is terminating
cns/middlewares/mock/mockClient.go	Adds mock data for testing MTPNC terminating state
cns/middlewares/k8sSwiftV2_linux_test.go	Adds test case to verify terminating state is properly handled

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

cns/middlewares/k8sSwiftV2_linux_test.go

Co-authored-by: Copilot <[email protected]> Signed-off-by: shreyashastantram <[email protected]>

miguelgoms · 2025-10-15T16:42:20Z

cns/middlewares/k8sSwiftV2.go

 	errGetMTPNC                 = errors.New(NetworkNotReadyErrorMsg + " - failed to get MTPNC")
 	errInvalidSWIFTv2NICType    = errors.New("invalid NIC type for SWIFT v2 scenario")
 	errInvalidMTPNCPrefixLength = errors.New("invalid prefix length for MTPNC primaryIP, must be 32")
+	errDeleteTimestampSet       = errors.New("MTPNC deletion timestamp is set")


these are still user-visible messages as this will be the error shown after "network not ready" to the cx on CNI
message should be more meaningful to the Cx - e.g.: "found mtpnc for previous pod pending deletion, waiting for new mtpnc to be ready"

arguably we could just return the "mtpncnotready" - but since the old mtpnc object is still visible, I think its better to have the separate error message.

this still applies to "mtpnc is terminating" - its not untrue, but it does not tell you why this is treated as a transient "network not ready" instead of a terminal error.

With that change, we are tagging this error with stateful set pod creation, if user removes ownership ref and tries deleting mtpnc for some reason, pod restart would result in the same error which is not the case. Yes, it's not the expected behavior but if user can do it, should we not consider that case? I want to keep it general so that it doesn't change with context

if user removes ownership and tries deleting before pod starts they'll get this error - which tells them this will not succeed without a new mtpnc on ready state. and if the mtpnc completes delete on the next CNI restart they'll get "mtpnc not ready"

isaac-dasan · 2025-10-16T21:58:32Z

cns/middlewares/k8sSwiftV2.go

 	errGetMTPNC                 = errors.New(NetworkNotReadyErrorMsg + " - failed to get MTPNC")
 	errInvalidSWIFTv2NICType    = errors.New("invalid NIC type for SWIFT v2 scenario")
 	errInvalidMTPNCPrefixLength = errors.New("invalid prefix length for MTPNC primaryIP, must be 32")
+	errMTPNCTerminating         = errors.New("MTPNC in terminating state")


Update to deleting && add NetworkNotReadyErrorMsg
Without NetworkNotReadyErrorMsg kubelet won't retry faster for the new pod. You can check my PR when I added NetworkNotReadyErrorMsg for the reasoning behind this.

Adding delete timestamp check

ab1235e

ashvindeodhar reviewed Oct 15, 2025

View reviewed changes

rbtr requested changes Oct 15, 2025

View reviewed changes

Adding UT and refactoring getmtpnc

5ccb46e

shreyashastantram marked this pull request as ready for review October 15, 2025 23:10

shreyashastantram requested review from a team as code owners October 15, 2025 23:10

shreyashastantram requested review from camrynl and Copilot October 15, 2025 23:10

Copilot AI reviewed Oct 15, 2025

View reviewed changes

cns/middlewares/k8sSwiftV2_linux_test.go Outdated Show resolved Hide resolved

shreyashastantram changed the title ~~Adding delete timestamp check~~ fix: Adding delete timestamp check Oct 15, 2025

shreyashastantram requested a review from rbtr October 15, 2025 23:19

Update cns/middlewares/k8sSwiftV2_linux_test.go

6274f99

Co-authored-by: Copilot <[email protected]> Signed-off-by: shreyashastantram <[email protected]>

miguelgoms reviewed Oct 16, 2025

View reviewed changes

isaac-dasan requested changes Oct 16, 2025

View reviewed changes

Changing the MTPNC error message

c2bbd84

shreyashastantram requested a review from isaac-dasan October 17, 2025 19:02

fix: Adding delete timestamp check #4078

Are you sure you want to change the base?

fix: Adding delete timestamp check #4078

Uh oh!

Conversation

shreyashastantram commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaac-dasan Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shreyashastantram commented Oct 14, 2025 •

edited

Loading

isaac-dasan Oct 16, 2025 •

edited

Loading