Skip to content

Conversation

@pjohnst5
Copy link
Contributor

@pjohnst5 pjohnst5 commented May 23, 2023

This is to make a third, consolidated, 'overlay' cns ipam mode

Once this is merged, we will do the following to safely migrate towards this option:

  • First, add third "overlay" option (keeps v4overlay and dualstack options still valid)
  • Switch dualstack to "overlay" in AKS-RP
  • Switch v4overlay to "overlay" in AKS-RP
  • Remove "v4overlay" and "dualstack" options in CNS

See our teams channel chat about this:

Ashvin:
Matthew Long do you see any issue with updating the the cns ipam mode in CNI conflist to just 'overlay' for v4 overlay?
Matthew Long
Nope no issue, maybe we’ll keep both for a little while in case of any rollbacks for backwards compatibility but eventually we can just remove the v4 part
Neha Aggarwal
I remember it was required to keep different handling in CNI too. Tamilmani Manoharan do you remember it?
Tamilmani Manoharan
Paul is right.. cni requires mode only to differentiate podsubnet or overlay but not needed for v4 and dualstack. instead of mode, based on ipaddress cni can detect if dualstack or not.. so just keeping overlay makes sense.. have to update cni code accordingly (shouldn't be a big change)...

@pjohnst5 pjohnst5 changed the title Making third 'overlay' option to cns ipam mode Making consolidated 'overlay' option to cns ipam mode May 23, 2023
@pjohnst5 pjohnst5 marked this pull request as ready for review June 6, 2023 18:25
@pjohnst5 pjohnst5 requested review from a team as code owners June 6, 2023 18:25
@pjohnst5 pjohnst5 force-pushed the paujohns/cnsoverlayipammode branch from 887376e to 89aa367 Compare June 6, 2023 23:51
@pjohnst5 pjohnst5 force-pushed the paujohns/cnsoverlayipammode branch from 1aadc13 to 16e537c Compare June 7, 2023 23:25
Copy link
Contributor

@paulyufan2 paulyufan2 left a comment

Choose a reason for hiding this comment

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

lgtm

CNI_OVERLAY_BUILD_DIR = $(BUILD_DIR)/cni-overlay
CNI_BAREMETAL_BUILD_DIR = $(BUILD_DIR)/cni-baremetal
CNI_DUALSTACK_BUILD_DIR = $(BUILD_DIR)/cni-dualstack
CNI_OVERLAY_CONSOLIDATED_BUILD_DIR = $(BUILD_DIR)/cni-overlay-consolidated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am opposed to adding a new build artifact like this, as we will then need to migrate off of it in the future and back to the bare "overlay" artifact.
Can we not simply consolidate on to "cni-overlay" immediately?

Copy link
Contributor Author

@pjohnst5 pjohnst5 Jun 8, 2023

Choose a reason for hiding this comment

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

I think that is a good point, one concern was backwards compatibility, to still have 'v4overlay' and 'dualstack' be valid options until we know 'overlay' works properly in AKS

So being able to roll back and still support 'dualstack' and 'v4overlay' was the major concern

Edit: Another reason was, to have some overlap in time, where CNS suports any overlay option, while we migrate off of 'v4overlay' and 'dualstackoverlay' in AKS

@ashvindeodhar wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thatmattlong what do you think as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rbtr what do you think, about having that overlap period in AKS (where all three options are viable)?

It maybe simpler to consolidate immediately, we would just need to keep these changes right in sync with AKS-RP changes and a new CNS version, might miss something

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was specifically referring to the tgz build artifacts, but looking further I think maybe too many things are changing in this single PR (or they should be reordered).
Should the changes actually be:

  • release CNI that supports the consolidated "executionMode":"swift" and ipam mode overlay
    • this should be end-to-end and should collapse v4/dualstack overlay in to the single cni-overlay build artifact
  • release dropgz with ^
  • release CNS that supports the consolidated overlay conflist generation
  • migrate AKS to consolidate overlay config argument everywhere
  • deprecate and remove v4overlay/dualstack overlay everywhere

vs the updates you have proposed, this way we should never need to update anything to an intermediate "overlay-consolidated" that we later need to change back.

Copy link
Contributor Author

@pjohnst5 pjohnst5 Jun 8, 2023

Choose a reason for hiding this comment

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

Oh I didn't know about "executionMode": "swift" conslidation, is there someone working on that currently?

release dropgz with ^

After releasing dropgz, would we update AKS-RP anywhere?
One change we would need to do for sure, is make dualstack overlay point to overlay conflist

release CNS that supports the consolidated overlay conflist generation

Would dualstack and v4overlay conflist generation be gone at this point completely? Or there, and not used?

Copy link
Contributor Author

@pjohnst5 pjohnst5 Jun 8, 2023

Choose a reason for hiding this comment

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

We update azure-cni binary and conflist at once, by virtue of tarball
> Check this new binary and conflist don't break V4
> Maybe add pipeline step for overlay cni
> what aks checks are there?
> Still would release dualstack version for backcompat

Relesae dropgz with ^
Rollout this everywhere, ~4 weeks (CNI is everywhere, and supports 'overlay' mode)
> Can make this faster by cutting new Agent baker image for dropgz right after tarball is release in our repo
Add a third 'overlay' generator, until cns image is rolled out (4 weeks)
Change the configmap, to use 'overlay'
Remove 'dualstack' and 'v4overlay' in CNS and CNI

Unknowns:
executionMode v4swift, what is it? Do we need it in v4overlay?

@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Jun 9, 2023

We're going to follow this strategy now to update to overlay
#2008

@pjohnst5 pjohnst5 closed this Jun 9, 2023
@pjohnst5 pjohnst5 deleted the paujohns/cnsoverlayipammode branch June 9, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants