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

Build and push multi-arch/os (amazon and windows, no debian) image manifest via Make rules #957

Merged
merged 2 commits into from
Aug 5, 2021

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Jul 1, 2021

Is this a bug fix or adding new feature? /feature

What is this PR about? / Why do we need it? Windows support merged recently but we haven't yet released a version with it. In preparation for that we need to build a multi-arch/os image manifest list with one of those manifests being a windows image.

The new Makefile copies heavily from the pause image Makefile which I found to have the prettiest patterns for achieving multi-arch build https://github.com/kubernetes/kubernetes/blob/v1.22.0-beta.0/build/pause/Makefile

I just made some minor adjustments for personal taste (like naming rules image instead of container). And, given a new enough docker cli we don't need to do the osversion workaround used there, we can just annotate manifests with argument --os-version.

In 1.2.0 release we decided to push only amazon linux based variant to GCR and ECR. So, this PR is also stopping the debian based build. (Decision came after sufficient debate which you can see by the silly amount of edits to this PR)

Before:

  • amazon based image
  • debian based image

After:

  • amazon based image
  • windows 1809 based image
  • windows 2004 based image
  • windows 20H2 based image

What testing is done?
build and push succeeded for my personal registry.

$ REGISTRY=280.dkr.ecr.us-west-2.amazonaws.com make all-push
...
...
...
$ docker manifest inspect 280.dkr.ecr.us-west-2.amazonaws.com/aws-ebs-csi-driver:asdf
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
   "manifests": [
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 953,
         "digest": "sha256:1b1745bccf3fb07ee963bca0b7b11c59d350535e61b3390995c8c571aa46bb3a",
         "platform": {
            "architecture": "amd64",
            "os": "linux",
            "os.version": "amazon"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 953,
         "digest": "sha256:6475d343ac68bc3c796b93910a21932e462829a909cd91009e68715d5b5cfabf",
         "platform": {
            "architecture": "arm64",
            "os": "linux",
            "os.version": "amazon"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 956,
         "digest": "sha256:835c8ef03568dd3e62412159be7b60a28ee15c4ff5ad9c56939119d2060a4858",
         "platform": {
            "architecture": "amd64",
            "os": "windows",
            "os.version": "1809"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 957,
         "digest": "sha256:42105b8b21835cb93f3bc6ed9e05a656043a20eab97eb0a3edb64bd965065dc2",
         "platform": {
            "architecture": "amd64",
            "os": "windows",
            "os.version": "2004"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 957,
         "digest": "sha256:70ca1715db49c844e503ca45a1cd237ae7ae0c905276d03758021f2600c47563",
         "platform": {
            "architecture": "amd64",
            "os": "windows",
            "os.version": "20H2"
         }
      }
   ]
}

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 1, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wongma7

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 1, 2021
@wongma7 wongma7 force-pushed the windowsbuild branch 6 times, most recently from 13293fc to fb44a43 Compare July 1, 2021 18:34
@wongma7
Copy link
Contributor Author

wongma7 commented Jul 1, 2021

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2021
@wongma7

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2021
@wongma7 wongma7 changed the title WIP: Build and push multi-arch/os (including windows) image manifest via Make rules Build and push multi-arch/os (including windows) image manifest via Make rules Jul 9, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Jul 13, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2021
@wongma7 wongma7 changed the title Build and push multi-arch/os (including windows) image manifest via Make rules Build and push multi-arch/os (amazon and windows, no debian) image manifest via Make rules Aug 3, 2021
@wongma7 wongma7 force-pushed the windowsbuild branch 2 times, most recently from 96109f2 to 593ecf3 Compare August 3, 2021 23:30
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2021
@wongma7 wongma7 force-pushed the windowsbuild branch 2 times, most recently from 824cf72 to edfe3f3 Compare August 3, 2021 23:43
GIT_COMMIT?=$(shell git rev-parse HEAD)
BUILD_DATE?=$(shell date -u +"%Y-%m-%dT%H:%M:%SZ")
BUILD_DATE?=$(shell date -u -Iseconds)

Choose a reason for hiding this comment

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

Any reason to not use human readable here?

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 is the same, just shows offest instead of Z, e.g. 2021-08-05T21:02:49+00:00

--progress=plain \
--target=$(OS)-$(OSVERSION) \
--output=type=$(OUTPUT_TYPE) \
-t=$(IMAGE):$(TAG)-$(OS)-$(ARCH)-$(OSVERSION) \

Choose a reason for hiding this comment

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

Is there a standard for this format of tag? If not we should make sure we come up with something across the provider-aws subprojects (i.e. just use the same format that you are using). This docker post shows a buildx example where the different platforms share a tag, but I'm not clear how pulling works in that case.

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 tag is only used for local build and adding the locally built image to the manifest list, it won't get pushed with the same tag as far as I can tell . At least, when I pushed it to public ECR as a test it did not expose these intermediary tags anywhere https://gallery.ecr.aws/b5w6x5z2/aws-ebs-csi-driver

Choose a reason for hiding this comment

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

Ah, makes sense 👍

@nckturner
Copy link

/lgtm

@khushboo121
Copy link

khushboo121 commented Oct 12, 2021

@wongma7 which helm chart version multi-arch/os changes are going in, as I am currently running the chart version: 2.3.0, but ebs-csi-node ds is not running on arm64 node:

NAME           DESIRED   CURRENT   READY   UP-TO-DATE   AVAILABLE   NODE SELECTOR            AGE
aws-node       31        31        31      31           31          <none>                   409d
ebs-csi-node   28        28        28      28           28          kubernetes.io/os=linux   263d
kube-proxy     31        31        31      31           31          <none>                   409d```

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. 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

4 participants