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.

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

chore: Polish node manager image building #974

Conversation

mainred
Copy link
Contributor

@mainred mainred commented Jan 6, 2022

What type of PR is this?

What this PR does / why we need it:

Changes included in this PR:

  • address part of arm64 support for Windows cloud node manager image and refine the dev doc #829, as arm64 windows image support cannot be achieved as mentioned as a limitation of arm64 support for the Windows nanoserver and servercore images.
  • unify both Linux and windows node images with the same repository name, like gcr.io/k8s-staging-provider-azure/azure-cloud-node-manager, and is distinguished by different tag naming patterns. Linux is with tag $TAG-linux-$ARCH, while windows are with tag $TAG-windows-$OSVERSION-$ARCH, for example:
qingchuanhao.azurecr.io/cloud-provider-azure/azure-cloud-node-manager:v1.1.3-linux-amd64
qingchuanhao.azurecr.io/cloud-provider-azure/azure-cloud-node-manager:v1.1.3-windows-1809-amd64

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

null

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 6, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @mainred. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 6, 2022
@coveralls
Copy link

coveralls commented Jan 6, 2022

Coverage Status

Coverage remained the same at 79.954% when pulling 19b0e43 on mainred:polish-node-manager-image-building into e3c0259 on kubernetes-sigs:master.

Copy link
Contributor

@zarvd zarvd left a comment

Choose a reason for hiding this comment

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

And I notice that azurefile-csi-driver Makefile is tidier and more readable. Maybe we could borrow something from that.

Makefile Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 7, 2022
@mainred mainred changed the title WIP: Polish node manager image building Polish node manager image building Jan 7, 2022
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 7, 2022
@@ -102,7 +107,7 @@ $(BIN_DIR)/azure-cloud-node-manager: $(PKG_CONFIG) $(wildcard cmd/cloud-node-man
CGO_ENABLED=0 GOOS=linux GOARCH=${ARCH} go build -a -o $(BIN_DIR)/azure-cloud-node-manager $(shell cat $(PKG_CONFIG)) ./cmd/cloud-node-manager

$(BIN_DIR)/azure-cloud-node-manager.exe: $(PKG_CONFIG) $(wildcard cmd/cloud-node-manager/*) $(wildcard cmd/cloud-node-manager/**/*) $(wildcard pkg/**/*) ## Build node-manager binary for Windows.
CGO_ENABLED=0 GOOS=windows go build -a -o $(BIN_DIR)/azure-cloud-node-manager.exe $(shell cat $(PKG_CONFIG)) ./cmd/cloud-node-manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Makefile target should be $(BIN_DIR)/azure-cloud-node-manager-${ARCH}.exe. Or we don't need the ARCH suffix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested the target respectively by WINDOWS_OSVERSION=1809 ARCH=arm64 make build-node-image-windows with output

CGO_ENABLED=0 GOOS=windows GOARCH=arm64 go build -a -o bin/azure-cloud-node-manager-arm64.exe -ldflags '-s -w -X sigs.k8s.io/cloud-provider-azure/pkg/version.gitVersion=v1.23.0-23-g2ef2d846d -X sigs.k8s.io/cloud-provider-azure/pkg/version.gitCommit=2ef2d846d7ecb982a02d9ce82087ac09acf3badb -X sigs.k8s.io/cloud-provider-azure/pkg/version.gitMajor=1 -X sigs.k8s.io/cloud-provider-azure/pkg/version.gitMinor=23+ -X sigs.k8s.io/cloud-provider-azure/pkg/version.buildDate=2022-01-06T14:52:13Z' ./cmd/cloud-node-manager

and

WINDOWS_OSVERSION=1809 ARCH=amd64 make build-node-image-windows with output

CGO_ENABLED=0 GOOS=windows GOARCH=amd64 go build -a -o bin/azure-cloud-node-manager-amd64.exe -ldflags '-s -w -X sigs.k8s.io/cloud-provider-azure/pkg/version.gitVersion=v1.23.0-23-g2ef2d846d -X sigs.k8s.io/cloud-provider-azure/pkg/version.gitCommit=2ef2d846d7ecb982a02d9ce82087ac09acf3badb -X sigs.k8s.io/cloud-provider-azure/pkg/version.gitMajor=1 -X sigs.k8s.io/cloud-provider-azure/pkg/version.gitMinor=23+ -X sigs.k8s.io/cloud-provider-azure/pkg/version.buildDate=2022-01-06T14:52:13Z' ./cmd/cloud-node-manager

Copy link
Contributor

Choose a reason for hiding this comment

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

What i mean is that target should be the name of the binary. (ref: https://www.gnu.org/software/make/manual/html_node/Rule-Introduction.html)

And as the linux version $(BIN_DIR)/azure-cloud-node-manager does not suffix with ARCH...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The doc says usually. Don't you think that will give more complexity to the target?

A target is usually the name of a file that is generated by a program

word-hyphen = $(word $2,$(subst -, ,$1))

push-node-image-linux-%:
$(MAKE) ARCH=$* push-node-image-linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could remove such alias commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The makefile target is really messy previous, and I tried to remove as many of them as possible and leave the ones exposed to customers more clear.

We want to make the Linux node images publishing easier from the customer side by calling, they don't have to do an iteration

push-all-node-images-linux: $(addprefix push-node-image-linux-,$(ALL_ARCH.linux))

push-node-image-linux-%:
$(MAKE) ARCH=$* push-node-image-linux

push-node-image-windows-%:
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

Makefile Show resolved Hide resolved
docs/image-building.md Outdated Show resolved Hide resolved
docs/image-building.md Outdated Show resolved Hide resolved
@nilo19 nilo19 closed this Jan 9, 2022
@nilo19 nilo19 reopened this Jan 9, 2022
@nilo19
Copy link
Contributor

nilo19 commented Jan 9, 2022

/ok-to-test
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 9, 2022
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 9, 2022
@feiskyer
Copy link
Member

@mainred the changes have broken image building in CIs. could you fix it?

@mainred
Copy link
Contributor Author

mainred commented Jan 10, 2022

Fix is on the way.

@mainred
Copy link
Contributor Author

mainred commented Jan 10, 2022

/retest

pull-cloud-provider-azure-e2e-ccm-capz failed for authorization failure when pushing windows image to capz registry
pull-cloud-provider-azure-e2e failed for a closed watch by a potential network failure.

@mainred
Copy link
Contributor Author

mainred commented Jan 10, 2022

/retest

1 similar comment
@mainred
Copy link
Contributor Author

mainred commented Jan 11, 2022

/retest

@mainred mainred changed the title Polish node manager image building chore: Polish node manager image building Jan 11, 2022
@mainred mainred force-pushed the polish-node-manager-image-building branch from 8301d3e to 19b0e43 Compare January 11, 2022 08:38
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, mainred, nilo19

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feiskyer
Copy link
Member

Thanks for the improvement and adding the docs.

@k8s-ci-robot k8s-ci-robot merged commit 4555148 into kubernetes-sigs:master Jan 11, 2022
@mainred mainred mentioned this pull request Jan 12, 2022
mainred pushed a commit to mainred/cloud-provider-azure that referenced this pull request Jan 12, 2022
…er-image-building

chore: Polish node manager image building
mainred pushed a commit to mainred/cloud-provider-azure that referenced this pull request Jan 12, 2022
…er-image-building

chore: Polish node manager image building
mainred pushed a commit to mainred/cloud-provider-azure that referenced this pull request Jan 12, 2022
…er-image-building

chore: Polish node manager image building
k8s-ci-robot added a commit that referenced this pull request Jan 13, 2022
…ng-release-1.1

cherry-pick of #974 to release-1.1: chore: Polish node manager image building
k8s-ci-robot added a commit that referenced this pull request Jan 13, 2022
…ng-release-0.7

cherry-pick of #974 to release-0.7: chore: Polish node manager image building
k8s-ci-robot added a commit that referenced this pull request Jan 13, 2022
…ng-release-1.0

cherry-pick of #974 to release-1.0: chore: Polish node manager image building
nilo19 added a commit that referenced this pull request Jan 20, 2022
…-1.23

[cherry-pick #974 to 1.23] Polish node manager image building
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants