Skip to content

Conversation

@ccchenjiahuan
Copy link
Contributor

@ccchenjiahuan ccchenjiahuan commented Aug 13, 2022

This pr has some notable changes:

  • Remove the webhook base image
  • Remove installer/volcano-development-arm64.yaml and arm specified images , because we don't need them anymore

Signed-off-by: ccchenjiahuan [email protected]

@volcano-sh-bot volcano-sh-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 13, 2022
@ccchenjiahuan ccchenjiahuan force-pushed the multi-arch branch 3 times, most recently from 5155e95 to d2f159e Compare August 13, 2022 08:36
@ccchenjiahuan
Copy link
Contributor Author

ccchenjiahuan commented Aug 13, 2022

The tested multi-arch images, it works fine in my local envs:

  • cccccccjh/vc-webhook-manager:cfdb71617741d657486293f751b0910cd28d6306
  • cccccccjh/vc-scheduler:cfdb71617741d657486293f751b0910cd28d6306
  • cccccccjh/vc-controller-manager:cfdb71617741d657486293f751b0910cd28d6306

The CI seems that the docker in minikube doesn't support buildkit? l tried but cannot figure it out, anyone knows how to avoid this?

image links: https://hub.docker.com/u/cccccccjh

@Thor-wl Thor-wl requested review from Thor-wl and shinytang6 and removed request for huone1 August 15, 2022 00:50
Copy link
Member

@shinytang6 shinytang6 left a comment

Choose a reason for hiding this comment

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

The implementations LGTM overall. Need to pass the CI before merging

@shinytang6
Copy link
Member

The tested multi-arch images, it works fine in my local envs:

  • cccccccjh/vc-webhook-manager:cfdb71617741d657486293f751b0910cd28d6306
  • cccccccjh/vc-scheduler:cfdb71617741d657486293f751b0910cd28d6306
  • cccccccjh/vc-controller-manager:cfdb71617741d657486293f751b0910cd28d6306

The CI seems that the docker in minikube doesn't support buildkit? l tried but cannot figure it out, anyone knows how to avoid this?

image links: https://hub.docker.com/u/cccccccjh

Yes, seems that docker buildx command is not supported in minikube.
@Thor-wl @william-wang @hwdef @Yikun Any ideas about the CI E2E Spark Integration Test case failure?

@Yikun
Copy link
Member

Yikun commented Aug 15, 2022

If we want to support the buildx, we need install buildx to support on spark e2e test like: #2438

@Yikun
Copy link
Member

Yikun commented Aug 15, 2022

@ccchenjiahuan Thanks for your work!

Feel free to squash #2438 in your pr, I did a e2e test with spark + buildx installation: #2438 , it works!

@Yikun
Copy link
Member

Yikun commented Aug 15, 2022

and E2E Sequence Jobs / E2E about Sequence (pull_request) Failing after 32m seems failed due to flaky test maybe @Thor-wl could do a double confirm

@Yikun
Copy link
Member

Yikun commented Aug 15, 2022

I also do a test manually in an arm64 env (apple M1):

$ minikube start --cpus 4 --memory 6144
$ eval $(minikube docker-env)
$ make TAG=latest update-development-yaml
$ make TAG=latest images
$ k apply -f ~/code/volcano/installer/volcano-development.yaml

$ docker inspect volcanosh/vc-scheduler | grep Architecture
        "Architecture": "arm64",
$ docker inspect volcanosh/vc-controller-manager | grep Architecture
        "Architecture": "arm64",
$ docker inspect volcanosh/vc-webhook-manager | grep Architecture
        "Architecture": "arm64",

$ k get pod -A | grep volcano
volcano-system   volcano-admission-56d746f6d-kt4rd                        1/1     Running     0             35m
volcano-system   volcano-admission-init-5lnbt                             0/1     Completed   0             35m
volcano-system   volcano-controllers-7c79f98f4-5jxk2                      1/1     Running     0             35m
volcano-system   volcano-scheduler-598bfd9df4-bgdmr                       1/1     Running     0             35m

Spark e2e test also work:

$ build/sbt -Pvolcano -Pkubernetes -Pkubernetes-integration-tests -Dspark.kubernetes.test.namespace=default -Dspark.kubernetes.test.deployMode=minikube -Dtest.include.tags=volcano "kubernetes-integration-tests/test"
Use 'docker scan' to run Snyk tests against images to find vulnerabilities and learn how to fix them
[info] KubernetesSuite:
[info] VolcanoSuite:
[info] - Run SparkPi with volcano scheduler (9 seconds, 380 milliseconds)
[info] - SPARK-38187: Run SparkPi Jobs with minCPU (21 seconds, 522 milliseconds)
[info] - SPARK-38187: Run SparkPi Jobs with minMemory (23 seconds, 454 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (only 1 enabled) (13 seconds, 350 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (all enabled) (22 seconds, 332 milliseconds)
[info] - SPARK-38423: Run driver job to validate priority order (16 seconds, 299 milliseconds)
[info] Run completed in 1 minute, 52 seconds.
[info] Total number of tests run: 6
[info] Suites: completed 2, aborted 0
[info] Tests: succeeded 6, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 177 s (02:57), completed 2022-8-15 17:23:24

@Yikun
Copy link
Member

Yikun commented Aug 15, 2022

BTW, this might bring some behavior changes:

  • Drop the base image
  • Replace k apply -f installer/volcano-development-arm64.yaml with k apply -f installer/volcano-development.yaml
  • Drop the arm specific image
  • Release process cc @Thor-wl

We'd better note in doc, release note or somewhere to mention them.

And pls also fullfill the PR description to explain all user face behavior changes. : )

Signed-off-by: ccchenjiahuan <[email protected]>
Copy link
Contributor

@Thor-wl Thor-wl left a comment

Choose a reason for hiding this comment

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

Generally LGTM for me. Looking forward to other reviewers' advices.

Copy link
Member

@shinytang6 shinytang6 left a comment

Choose a reason for hiding this comment

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

Thanks, also LGTM

@shinytang6
Copy link
Member

Another thing to check is whether the CI Daily Release work after merged

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2022
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Thor-wl

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

The pull request process is described here

Details 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

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2022
@volcano-sh-bot volcano-sh-bot merged commit 910a97d into volcano-sh:master Aug 17, 2022
@Yikun
Copy link
Member

Yikun commented Aug 17, 2022

And also changes the doc https://volcano.sh/en/docs/installation/#install-with-yaml-files

@Thor-wl
Copy link
Contributor

Thor-wl commented Aug 17, 2022

And also changes the doc https://volcano.sh/en/docs/installation/#install-with-yaml-files

Yes. @ccchenjiahuan Can you submit another PR to update the doc?

@hwdef
Copy link
Member

hwdef commented Aug 18, 2022

I tested this on m2 macbook air and and an amd64 virtual machine. It can work.

@shinytang6
Copy link
Member

@SoutherLea
Copy link

With this solution, each component will download the project's dependency package when it is built. If for some well-known reason, the download can easily fail due to network instability. In this case, it is difficult to successfully build images. :-P

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. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants