Skip to content

Conversation

@rbtr
Copy link
Collaborator

@rbtr rbtr commented Nov 1, 2023

Reason for Change:

#1767 introduced a bug where, if the NC in state does not have the Swift_ prefix, when we unconditionally add the prefix we become unable to identify it in the NMA responses which don't have it and are stuck:

{"httpStatusCode":"200","networkContainers":[{"networkContainerId":"de043d23-a135-401c-b6a8-1ab6a09f980a","version":"0"}]}

and yet

[Azure CNS] Failed to get NC de043d23-a135-401c-b6a8-1ab6a09f980a doesn't exist in NMAgent NC version list Skipping GetNCVersionStatus check from NMAgent

After investigating, this it seems like nobody knows where or even if we actually have NCs in the wild with the Swift_ prefix, but it is possible that we do as a legacy artifact, and it could be unsafe to remove the prefixing entirely.

However, for the purposes of the lookup in the NC version list query, we can strip the prefix when add items to this in-memory map, and strip prefixes when we attempt to look objects up in the map, and unstick ourselves. This additional prefixing is not done anywhere else, and no usages of these objects escape this local map, so it should be safe and back-compat.

Issue Fixed:

Requirements:

Notes:

@rbtr rbtr requested a review from a team as a code owner November 1, 2023 21:57
@rbtr rbtr requested a review from thatmattlong November 1, 2023 21:57
@rbtr rbtr force-pushed the fix/swift-prefix branch from 317c6f9 to 7e7376c Compare November 1, 2023 22:02
@rbtr rbtr self-assigned this Nov 1, 2023
@rbtr rbtr added cns Related to CNS. swift Related to SWIFT networking. fix Fixes something. multitenancy release/latest Change affects latest release train labels Nov 1, 2023
@rbtr rbtr force-pushed the fix/swift-prefix branch from 7e7376c to a962300 Compare November 2, 2023 15:18
Copy link
Member

@timraymond timraymond left a comment

Choose a reason for hiding this comment

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

Part of me wants to encapsulate all that NCID wrangling into some type or function that just contains the knowledge of wrangling prefixed and non-prefixed keys... but this does seem pretty isolated, so I'm okay with it. If we do anything more in this space though, I think it will become worth it.

@rbtr
Copy link
Collaborator Author

rbtr commented Nov 2, 2023

I agree. I went this way because maybe it's possible on extant deployments to have Swift_ prefixed NCIDs in CNS internal state (on disk) or in the communication path from DNC (existing CRD definitions). Finding all the data boundaries where those need to be transformed would blow this up into a much bigger change 🙁
I think we'll have to try to do that as a non backcompat change in the future and leave any old defs behind.

@rbtr
Copy link
Collaborator Author

rbtr commented Nov 2, 2023

/azp run

@rbtr rbtr enabled auto-merge (squash) November 2, 2023 21:15
@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@rbtr rbtr force-pushed the fix/swift-prefix branch from a962300 to 004f003 Compare November 2, 2023 21:48
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
@rbtr rbtr force-pushed the fix/swift-prefix branch from 004f003 to 050c8f3 Compare November 3, 2023 14:50
@rbtr rbtr merged commit de8f045 into master Nov 3, 2023
@rbtr rbtr deleted the fix/swift-prefix branch November 3, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cns Related to CNS. fix Fixes something. multitenancy release/latest Change affects latest release train swift Related to SWIFT networking.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants