Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: build cni installer image with cni builds #2324

Merged
merged 1 commit into from Oct 30, 2023
Merged

feat: build cni installer image with cni builds #2324

merged 1 commit into from Oct 30, 2023

Conversation

rbtr
Copy link
Contributor

@rbtr rbtr commented Oct 25, 2023 •
edited

Reason for Change:

Up until now, we have released the CNI installer image as a separately versioned component from the CNI releases themselves. This has created a bit of versioning hell, as we struggle to maintain multiple release trains of the CNI (and thus CNI installer) when the CNI and installer version are not correlated.

This change flips the CNI installer image builder architecture. Previously, dropgz pulled in the released CNI tarballs as dependencies and produced a "dropgz-vX" omnibus image which contained uncorrelated CNI versions.
Here, we migrate instead to a CNI Dockerfile which imports dropgz as the dependency and is built in lockstep with (at least) azure-vnet builds, and versioned the same as them. Instead of deploying a "dropgz:v0.0.4" with unknown (at least, non-obvious) CNI payload versions, we produce a "cni-installer:v1.5.11" during the CNI release process with no ambiguity.

This is part 1 of this migration:

  • Build CNI installer images
  • Build azure-ipam installer images
  • Set up MCR publishing and syndication on new images
  • Migrate pipelines to new images
  • Migrate downstreams (AKS) to new images
  • Remove existing dropgz image infra (Dockerfiles, pipelines, etc)

Issue Fixed:

Requirements:

Notes:

@rbtr rbtr added enhancement cni Related to CNI. release/1.4 Change affects v1.4 release train release/latest Change affects latest release train needs-backport Change needs to be backported to previous release trains dropgz dropgz labels Oct 25, 2023
@rbtr rbtr requested a review from a team as a code owner October 25, 2023 22:50
@rbtr rbtr self-assigned this Oct 25, 2023
@rbtr rbtr requested a review from a team as a code owner October 25, 2023 22:50
@rbtr rbtr requested a review from paulyufan2 as a code owner October 25, 2023 23:00
@rbtr rbtr requested a review from tyler-lloyd October 25, 2023 23:03
@rbtr rbtr force-pushed the fix/dropgz-cni-versions branch 3 times, most recently from 59fde34 to 27a0712 Compare October 27, 2023 16:38
@pjohnst5
Copy link
Collaborator

Just a qq, what does "omnibus" mean in this context? Sounds cool

"Previously, dropgz pulled in the released CNI tarballs as dependencies and produced a "dropgz-vX" omnibus image which contained uncorrelated CNI versions"

pjohnst5
pjohnst5 reviewed Oct 27, 2023
View reviewed changes
.pipelines/pipeline.yaml Outdated Show resolved Hide resolved
pjohnst5
pjohnst5 reviewed Oct 27, 2023
View reviewed changes
cni/linux.Dockerfile Show resolved Hide resolved
cni/linux.Dockerfile
COPY . .
RUN GOOS=$OS CGO_ENABLED=0 go build -a -o /go/bin/azure-vnet -trimpath -ldflags "-X main.version="$VERSION"" -gcflags="-dwarflocationlists=true" cni/network/plugin/main.go
RUN GOOS=$OS CGO_ENABLED=0 go build -a -o /go/bin/azure-vnet-telemetry -trimpath -ldflags "-X main.version="$VERSION"" -gcflags="-dwarflocationlists=true" cni/telemetry/service/telemetrymain.go
RUN GOOS=$OS CGO_ENABLED=0 go build -a -o /go/bin/azure-vnet-ipam -trimpath -ldflags "-X main.version="$VERSION"" -gcflags="-dwarflocationlists=true" cni/ipam/plugin/main.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

So does this include azure-vnet-ipam then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does not

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh interesting, how is that? What is this /go/bin/azure-vnet-ipam binary? How does it differ from the azure-vnet-ipam?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's the legacy (v1) ipam plugin for azure-vnet

cni/linux.Dockerfile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@rbtr rbtr mentioned this pull request Oct 27, 2023
Open
14 tasks
@rbtr rbtr linked an issue Oct 27, 2023 that may be closed by this pull request
CNI Installer migration #2333
Open
14 tasks
@rbtr rbtr force-pushed the fix/dropgz-cni-versions branch 2 times, most recently from c4e808b to 07797dd Compare October 27, 2023 18:54
@rbtr
Copy link
Contributor Author

rbtr commented Oct 27, 2023

@camrynl @vipul-21 @pjohnst5
I have updated this change to be additive only - creating the new image and infra for that. No changes to existing dropgz infra/tests at this time. This linked tracking issue describes the plan for building out this change and migrating to it.

@rbtr rbtr enabled auto-merge (squash) October 27, 2023 19:02
@jpayne3506
Copy link
Contributor

Remove existing dropgz image infra (Dockerfiles, pipelines, etc)

What is the expected impact from using older infra as this migration happens?

jpayne3506
jpayne3506 reviewed Oct 27, 2023
View reviewed changes
Makefile Show resolved Hide resolved
.pipelines/pipeline.yaml
@@ -159,6 +159,20 @@ stages:
name: "$(BUILD_POOL_NAME_LINUX_AMD64)"
strategy:
matrix:
cni_linux_amd64:
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless this gets renamed back to cni-dropgz, which from the PR description it is indicating that it will be,

Instead of deploying a "dropgz:v0.0.4" with unknown (at least, non-obvious) CNI payload versions, we produce a "cni-dropgz:v1.5.11"

we need to add this to the cleanup script regex for acnpublic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will follow up

@rbtr
feat: build cni installer image with cni builds
3ea0176 
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
@rbtr rbtr disabled auto-merge October 30, 2023 21:09
@rbtr rbtr merged commit 7e5994d into master Oct 30, 2023
82 checks passed
@rbtr rbtr deleted the fix/dropgz-cni-versions branch October 30, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpayne3506 jpayne3506 jpayne3506 approved these changes

@camrynl camrynl camrynl approved these changes

@ashvindeodhar ashvindeodhar Awaiting requested review from ashvindeodhar ashvindeodhar is a code owner automatically assigned from Azure/acn-cni-reviewers

@thatmattlong thatmattlong Awaiting requested review from thatmattlong

@paulyufan2 paulyufan2 Awaiting requested review from paulyufan2

@tyler-lloyd tyler-lloyd Awaiting requested review from tyler-lloyd

@pjohnst5 pjohnst5 Awaiting requested review from pjohnst5

@vipul-21 vipul-21 Awaiting requested review from vipul-21

Assignees

@rbtr rbtr

Labels
cni Related to CNI. dropgz dropgz enhancement needs-backport Change needs to be backported to previous release trains release/latest Change affects latest release train release/1.4 Change affects v1.4 release train
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CNI Installer migration
5 participants
@rbtr @pjohnst5 @jpayne3506 @vipul-21 @camrynl