DNM: Combine ceph-pull-requests and API jobs #1657

djgalloway · 2020-09-15T13:23:07Z

A couple notes:

I'm open to changing the job name (friendly name and otherwise)
I'm just disabling the existing/separate make check & API jobs for now. Will delete in the future if we're happy with the new "pipeline"

ceph-pr-combined/config/definitions/ceph-pr-combined.yml

epuertat

Thanks a lot for this quick PR, @djgalloway ! Looks perfect to me. Just added a few observations, but nothing blocking.

epuertat · 2020-09-15T17:04:45Z

ceph-pr-combined/config/definitions/ceph-pr-combined.yml

+          shallow-clone: true
+          wipe-workspace: true
+
+    ## TODO: (maybe) I'm sure there's a cleaner/prettier way to do this.  Maybe doing an actual Jenkins pipeline.


I think this is a great improvement in terms of resource usage, and Jenkins pipelines will definitely improve also the reporting of the different stages. If you need any help for a next PR, I'll gladly volunteer for helping bring pipelines into Ceph.

ceph-pr-combined/config/definitions/ceph-pr-combined.yml

epuertat · 2020-09-15T17:13:49Z

ceph-pr-combined/config/definitions/ceph-pr-combined.yml

+- job:
+    name: ceph-pr-combined
+    description: 'make check and ceph API tests combined'
+    project-type: freestyle


Not sure if this would imply much change to everything else, but we might keep the Jenkins Job Builder YAML syntax while introducing project-type: pipeline...

Yeah, I'm sure in the future we can streamline this job by making pipeline. The two troubles I had with that were

Ensuring all steps run on the same machine

Having two separate "checks" in GitHub and having them get updated accordingly

This will run `make check` and then the Ceph API tests against the built binaries instead of us running `make` twice per PR then running API tests against one of those runs. Signed-off-by: David Galloway <[email protected]>

Replaced by ceph-pr-combined Signed-off-by: David Galloway <[email protected]>

callithea

Lgtm, thanks a lot, @djgalloway!

epuertat · 2020-09-16T08:48:08Z

@djgalloway as this is causing some fuss in other teams, for the soak testing period, can we just make it run for dashboard-labeled PRs? Thanks!

djgalloway · 2020-09-16T12:56:19Z

@djgalloway as this is causing some fuss in other teams, for the soak testing period, can we just make it run for dashboard-labeled PRs? Thanks!

Or why don't we just merge as-is and tweak it after the fact?

djgalloway · 2020-09-17T12:52:30Z

@jdurgin @neha-ojha are y'all okay with merging this PR?

This will disable the ceph-pull-requests and ceph-pr-api jobs in Jenkins. It will change the GitHub Check names to ceph PR - make check and ceph PR - API tests.

The API tests require running make before them so we were essentially running make twice for each PR wasting time and resources.

epuertat · 2020-09-17T17:34:03Z

@djgalloway something I just realized: and I don't wanna sound like a killjoy, but it'll 'slightly' increment the time taken to finish (10-15 min), as here make check unit tests and API tests are running sequentially. I guess there's no way to run jobs in parallel without saving artifacts?

djgalloway · 2020-09-17T18:54:18Z

@djgalloway something I just realized: and I don't wanna sound like a killjoy, but it'll 'slightly' increment the time taken to finish (10-15 min), as here make check unit tests and API tests are running sequentially. I guess there's no way to run jobs in parallel without saving artifacts?

I'm sure there probably is. I'm guessing that's where the Pipeline would come in handy. So it'd be:

Phase 1: make
Phase 2 and 3 (simultaneous): Unit Tests and API tests ?

jdurgin · 2020-09-17T21:47:06Z

Sounds good to go to me. We can further improve in future PRs. Reducing the resources by nearly half seems worth merging sooner than later to me.

neha-ojha · 2020-09-17T21:55:38Z

@jdurgin @neha-ojha are y'all okay with merging this PR?

I have no objections.

This will disable the ceph-pull-requests and ceph-pr-api jobs in Jenkins. It will change the GitHub Check names to ceph PR - make check and ceph PR - API tests.

The API tests require running make before them so we were essentially running make twice for each PR wasting time and resources.

djgalloway · 2020-09-17T22:23:26Z

Alright, well, I'm on PTO tomorrow so Monday I guess.

epuertat · 2020-09-21T08:59:10Z

@djgalloway just started to experiment with pipelines (ceph/ceph#37265) and it seems it might not require too much rework...

djgalloway · 2020-09-22T14:39:20Z

@djgalloway just started to experiment with pipelines (ceph/ceph#37265) and it seems it might not require too much rework...

Okay. Are we going that route instead, then?

epuertat · 2020-09-22T20:07:14Z

Okay. Are we going that route instead, then?

I guess the multibranch pipeline should be enable on ceph/ceph, and that configuration could still be a jenkins-job-builder YAML file, right? I personally hate the groovy-based syntax of the Jenkinsfile, but I've seen a couple of projects ([1], [2]) to bring YAML support to them.

What do you think?

BTW, I created a pipeline job (epuertat-test-ceph-pipeline), but was... somehow deleted...

djgalloway · 2020-09-22T22:28:12Z

Okay. Are we going that route instead, then?

I guess the multibranch pipeline should be enable on ceph/ceph, and that configuration could still be a jenkins-job-builder YAML file, right? I personally hate the groovy-based syntax of the Jenkinsfile, but I've seen a couple of projects ([1], [2]) to bring YAML support to them.

What do you think?

My understanding is we'll need a small yaml file defining a pipeline job but that will pull in a groovy Jenkinsfile from ceph.git and ceph-ci.git. I'm fine with that if we can get it working. I didn't feel confident enough in my coding abilities to convert the ceph-pull-requests and ceph-api jobs into groovy so I did it this way.

BTW, I created a pipeline job (epuertat-test-ceph-pipeline), but was... somehow deleted...

Anytime something gets merged into https://github.com/ceph/ceph-build, a Jenkins job runs to update itself and deleted any jobs that aren't defined in this repo.

I would suggest logging in to Jenkins, getting an API key, and using jenkins-job-builder manually to test and upload your JJB YAMLs.

$ cat ~/.jenkins_jobs.jenkins.ceph.com.ini 
[jenkins]
user=GITHUB USERNAME
password=API KEY
url=https://jenkins.ceph.com

Here's the command I used to create/update/test/etc. the ceph-pr-combined job: jenkins-jobs --ignore-cache --conf ~/.jenkins_jobs.jenkins.ceph.com.ini update ceph-pr-combined/config/definitions/ceph-pr-combined.yml

epuertat · 2020-09-24T08:57:01Z

I would suggest logging in to Jenkins, getting an API key, and using jenkins-job-builder manually to test and upload your JJB YAMLs.
$ cat ~/.jenkins_jobs.jenkins.ceph.com.ini 
[jenkins]
user=GITHUB USERNAME
password=API KEY
url=https://jenkins.ceph.com

Thanks a lot @djgalloway ! Will try it and let you know.

djgalloway · 2021-05-20T19:29:07Z

This can't be merged as-is anymore. Needs a rebase.

djgalloway · 2022-04-01T18:37:01Z

@epuertat it pains me to still see us building Ceph binaries twice for every PR. What do you think about merging this next week?

epuertat · 2022-04-04T12:13:56Z

@epuertat it pains me to still see us building Ceph binaries twice for every PR. What do you think about merging this next week?

@djgalloway omd, completely forgot about this one! I'm totally supportive of this (as a later improvement, we should conditionally include the 'dashboard e2e' tests too). Do you have any recent run of this, to verify it works ok?

djgalloway · 2022-04-04T13:55:12Z

Do you have any recent run of this, to verify it works ok?

Good call. Will test today.

djgalloway requested review from epuertat and callithea September 15, 2020 13:24

callithea reviewed Sep 15, 2020

View reviewed changes

ceph-pr-combined/config/definitions/ceph-pr-combined.yml Outdated Show resolved Hide resolved

djgalloway force-pushed the wip-make-api-pipeline branch from 6c2e24e to 064559a Compare September 15, 2020 14:49

epuertat requested a review from alfonsomthd September 15, 2020 14:54

djgalloway force-pushed the wip-make-api-pipeline branch 2 times, most recently from 56756b3 to c5a5dea Compare September 15, 2020 16:27

epuertat approved these changes Sep 15, 2020

View reviewed changes

David Galloway added 2 commits September 15, 2020 15:30

Create ceph-pr-combined job

ff8e05d

This will run `make check` and then the Ceph API tests against the built binaries instead of us running `make` twice per PR then running API tests against one of those runs. Signed-off-by: David Galloway <[email protected]>

Disable ceph-pr-api and ceph-pull-requests individual jobs

fc4121a

Replaced by ceph-pr-combined Signed-off-by: David Galloway <[email protected]>

djgalloway force-pushed the wip-make-api-pipeline branch from c5a5dea to fc4121a Compare September 15, 2020 19:30

callithea approved these changes Sep 16, 2020

View reviewed changes

djgalloway changed the title ~~Combine ceph-pull-requests and API jobs~~ DNM: Combine ceph-pull-requests and API jobs May 20, 2021

djgalloway changed the base branch from master to main May 25, 2022 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DNM: Combine ceph-pull-requests and API jobs #1657

DNM: Combine ceph-pull-requests and API jobs #1657

djgalloway commented Sep 15, 2020 •

edited

Loading

epuertat left a comment

epuertat Sep 15, 2020 •

edited

Loading

epuertat Sep 15, 2020

djgalloway Sep 15, 2020

epuertat Sep 16, 2020

callithea left a comment

epuertat commented Sep 16, 2020

djgalloway commented Sep 16, 2020

djgalloway commented Sep 17, 2020

epuertat commented Sep 17, 2020

djgalloway commented Sep 17, 2020

jdurgin commented Sep 17, 2020

neha-ojha commented Sep 17, 2020

djgalloway commented Sep 17, 2020

epuertat commented Sep 21, 2020

djgalloway commented Sep 22, 2020 •

edited

Loading

epuertat commented Sep 22, 2020

djgalloway commented Sep 22, 2020

epuertat commented Sep 24, 2020

djgalloway commented May 20, 2021

djgalloway commented Apr 1, 2022

epuertat commented Apr 4, 2022

djgalloway commented Apr 4, 2022

DNM: Combine ceph-pull-requests and API jobs #1657

Are you sure you want to change the base?

DNM: Combine ceph-pull-requests and API jobs #1657

Conversation

djgalloway commented Sep 15, 2020 • edited Loading

epuertat left a comment

Choose a reason for hiding this comment

epuertat Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

epuertat Sep 15, 2020

Choose a reason for hiding this comment

djgalloway Sep 15, 2020

Choose a reason for hiding this comment

epuertat Sep 16, 2020

Choose a reason for hiding this comment

callithea left a comment

Choose a reason for hiding this comment

epuertat commented Sep 16, 2020

djgalloway commented Sep 16, 2020

djgalloway commented Sep 17, 2020

epuertat commented Sep 17, 2020

djgalloway commented Sep 17, 2020

jdurgin commented Sep 17, 2020

neha-ojha commented Sep 17, 2020

djgalloway commented Sep 17, 2020

epuertat commented Sep 21, 2020

djgalloway commented Sep 22, 2020 • edited Loading

epuertat commented Sep 22, 2020

djgalloway commented Sep 22, 2020

epuertat commented Sep 24, 2020

djgalloway commented May 20, 2021

djgalloway commented Apr 1, 2022

epuertat commented Apr 4, 2022

djgalloway commented Apr 4, 2022

djgalloway commented Sep 15, 2020 •

edited

Loading

epuertat Sep 15, 2020 •

edited

Loading

djgalloway commented Sep 22, 2020 •

edited

Loading