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

[#580] chore: move deploy/kubernetes to a standalone workflow #578

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

kaijchen
Copy link
Contributor

@kaijchen kaijchen commented Feb 10, 2023

What changes were proposed in this pull request?

  1. Move kubernetes profile from parallel.yml to a standalone workflow.
  2. Speed up kubernetes workflow by skipping java tests.

Why are the changes needed?

Sub-task of #580

  1. Cleanup regular CI parallel.yml.
  2. Speed up kubernetes CI.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

CI.

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Merging #578 (17cb26a) into master (6692516) will decrease coverage by 12.33%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master     #578       +/-   ##
=============================================
- Coverage     61.13%   48.80%   -12.33%     
=============================================
  Files           213       14      -199     
  Lines         12328     1971    -10357     
  Branches       1041        0     -1041     
=============================================
- Hits           7537      962     -6575     
+ Misses         4382      949     -3433     
+ Partials        409       60      -349     
Impacted Files Coverage Δ
...org/apache/uniffle/server/LocalStorageChecker.java
.../java/org/apache/uniffle/common/util/RssUtils.java
...org/apache/uniffle/common/metrics/GRPCMetrics.java
...niffle/client/response/CompressedShuffleBlock.java
...niffle/common/exception/FileNotFoundException.java
...niffle/coordinator/strategy/storage/RankValue.java
...k/shuffle/writer/WrappedByteArrayOutputStream.java
...java/org/apache/uniffle/common/util/ExitUtils.java
...apache/uniffle/storage/common/ShuffleFileInfo.java
...niffle/common/rpc/MonitoringServerInterceptor.java
... and 189 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@advancedxy
Copy link
Contributor

Seems like other module is still compiled . it is possible to run build-operator.sh and test-operator.sh directly.
You can use mvn package -P kubernetes -am -pl deploy/kubernetes to just build the operator package.
And there's no need to skipTests here.

@advancedxy
Copy link
Contributor

advancedxy commented Feb 10, 2023

Anyway, many thanks for the work. I thought about it a while ago.

@jerqi
Copy link
Contributor

jerqi commented Feb 10, 2023

Seems like other module is still compiled . it is possible to run build-operator.sh and test-operator.sh directly. You can use mvn package -P kubernetes -am -pl deploy/kubernetes to just build the operator package. And there's no need to skipTests here.

We need other jars to run K8S integration tests, so we should compile them.

@advancedxy
Copy link
Contributor

Seems like other module is still compiled . it is possible to run build-operator.sh and test-operator.sh directly. You can use mvn package -P kubernetes -am -pl deploy/kubernetes to just build the operator package. And there's no need to skipTests here.

We need other jars to run K8S integration tests, so we should compile them.

Currently, there's no jars needed to run K8S tests? Even if we enable integration tests, we may relies the rss-server images to run integration tests?

@jerqi
Copy link
Contributor

jerqi commented Feb 10, 2023

Seems like other module is still compiled . it is possible to run build-operator.sh and test-operator.sh directly. You can use mvn package -P kubernetes -am -pl deploy/kubernetes to just build the operator package. And there's no need to skipTests here.

We need other jars to run K8S integration tests, so we should compile them.

Currently, there's no jars needed to run K8S tests? Even if we enable integration tests, we may relies the rss-server images to run integration tests?

We should use latest jars to produce image.

@advancedxy
Copy link
Contributor

We should use latest jars to produce image.

you mean deploy/kubernetes/docker/Dockerfile? I think it's not used in CI.

@jerqi
Copy link
Contributor

jerqi commented Feb 10, 2023

We should use latest jars to produce image.

you mean deploy/kubernetes/docker/Dockerfile? I think it's not used in CI.

Yes. why don't we use it in IT?

@advancedxy
Copy link
Contributor

We should use latest jars to produce image.

you mean deploy/kubernetes/docker/Dockerfile? I think it's not used in CI.

Yes. why don't we use it in IT?

We just don't docker build this Dockerfile yet.

@jerqi
Copy link
Contributor

jerqi commented Feb 10, 2023

We should use latest jars to produce image.

you mean deploy/kubernetes/docker/Dockerfile? I think it's not used in CI.

Yes. why don't we use it in IT?

We just don't docker build this Dockerfile yet.

I mean that we will use it if we have IT tests.

@advancedxy
Copy link
Contributor

We should use latest jars to produce image.

you mean deploy/kubernetes/docker/Dockerfile? I think it's not used in CI.

Yes. why don't we use it in IT?

We just don't docker build this Dockerfile yet.

I mean that we will use it if we have IT tests.

Ah, yes. But I think in most cases, we will just reuse the already existed docker image or put the shuffler server docker image
build process parallel with rss-controller/rss-webhook build process.

When we start integration tests, the ci workflow will need some refactor work to support it.

@kaijchen
Copy link
Contributor Author

kaijchen commented Feb 10, 2023

@jerqi @advancedxy Let's just create a new CI workflow for deploy and k8s tests later.
We can add -DskipUTs -DskipITs in the new workflow, and remove it from the kubernetes profile.

The parallel.yml should be used just for integration with different compute engines.

@advancedxy
Copy link
Contributor

@jerqi @advancedxy Let's just create a new CI workflow for deploy and k8s tests later. We can add -DskipUTs -DskipITs in the new workflow, and remove it from the kubernetes profile.

The parallel.yml should be used just for integration with different compute engines.

fair point..

pom.xml Outdated Show resolved Hide resolved
@jerqi
Copy link
Contributor

jerqi commented Feb 10, 2023

We should use latest jars to produce image.

you mean deploy/kubernetes/docker/Dockerfile? I think it's not used in CI.

Yes. why don't we use it in IT?

We just don't docker build this Dockerfile yet.

I mean that we will use it if we have IT tests.

Ah, yes. But I think in most cases, we will just reuse the already existed docker image or put the shuffler server docker image build process parallel with rss-controller/rss-webhook build process.

When we start integration tests, the ci workflow will need some refactor work to support it.

It's ok.

pom.xml Outdated Show resolved Hide resolved
@kaijchen kaijchen marked this pull request as draft February 10, 2023 05:25
@kaijchen kaijchen changed the title [MINOR] chore: skip java tests in kubernetes profile [#580] chore: skip java tests in kubernetes profile Feb 10, 2023
@kaijchen kaijchen marked this pull request as ready for review February 10, 2023 06:33
@kaijchen
Copy link
Contributor Author

@jerqi @advancedxy I've added a standalone workflow. How do you think?

@jerqi
Copy link
Contributor

jerqi commented Feb 10, 2023

It's ok for me.

@advancedxy
Copy link
Contributor

@jerqi @advancedxy I've added a standalone workflow. How do you think?

Sounds good to me.

uses: ./.github/workflows/deploy.yml
with:
maven-args: package
maven-options: -DskipUTs -DskipITs
Copy link
Contributor

Choose a reason for hiding this comment

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

for this case, I think it should be more related to the k8s profile?

For k8s, it doesn't need to run maven with uts..
however there could be other deploy options.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you call it k8s_deploy.yml ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be deploy/YARN in the future. Let's keep it as is now?
If you wish to not run it with maven, feel free to change it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

There might be deploy/YARN in the future. Let's keep it as is now? If you wish to not run it with maven, feel free to change it later.

I like the idea that we can simply call maven to build all the stuff. I am not intended to run without mvn.
I'am thinking about this case.

# for k8s
mvn -Pk8s -DskipUTs -DskipITs

# for yarn, it should be without -DskipUTs.
mvn -Pyarn

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you should use this trick to put maven options in the matrix side. https://stackoverflow.com/questions/66025220/paired-values-in-github-actions-matrix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# for yarn, it should be without -DskipUTs.
mvn -Pyarn

I don't want to support YARN in this PR.
YARN will be supported when it's introduced (probably never).

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 just want to do minimal changes to move k8s test out of parallel.yml.
And speed up k8s CI by skipping UT and IT.

@kaijchen kaijchen changed the title [#580] chore: skip java tests in kubernetes profile [#580] chore(ci): skip java tests in kubernetes profile Feb 10, 2023
@kaijchen kaijchen changed the title [#580] chore(ci): skip java tests in kubernetes profile [#580] chore: skip java tests in kubernetes profile Feb 10, 2023
@kaijchen kaijchen changed the title [#580] chore: skip java tests in kubernetes profile [#580] chore: add a deploy/kubernetes workflow and skip java tests Feb 10, 2023
@kaijchen kaijchen changed the title [#580] chore: add a deploy/kubernetes workflow and skip java tests [#580] chore: add a standalone workflow for deploy/kubernetes Feb 10, 2023
@kaijchen kaijchen changed the title [#580] chore: add a standalone workflow for deploy/kubernetes [#580] chore(CI): move deploy/kubernetes to a standalone workflow Feb 10, 2023
@kaijchen kaijchen changed the title [#580] chore(CI): move deploy/kubernetes to a standalone workflow [#580] chore: move deploy/kubernetes to a standalone workflow Feb 10, 2023
Copy link
Contributor

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

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

I'm ok to move the -DskipUTs ... into the deploy.yml side.

@kaijchen kaijchen merged commit f725149 into apache:master Feb 13, 2023
@kaijchen kaijchen deleted the operator-test branch February 13, 2023 08:28
@kaijchen
Copy link
Contributor Author

Thanks @advancedxy and @jerqi for the discussion and review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants