Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

S3 upload artifacts #16336

Merged
merged 6 commits into from
Oct 2, 2019
Merged

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Sep 30, 2019

Description

Upload nosetest artifacts such as nosetests_python2_cpu_quantization.xml, nosetests_python2_cputrain_.xml and nosetests_python2_cpu_unittest.xml for every job (once it is merged on the master branch)

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • ci/Jenkins_utils.groovy
  • Add S3 upload plugin to the jenkins-mxnet-ci.amazon.com;
  • Allow read/write access to the specific S3 bucket (for Jenkins job to be able to upload to the S3 bucket
  • To maintain security, S3 bucket name is added as a Global property (like an environment variable of sorts) inside the Jenkins -> Configure

Test

Test all the above changes if they work on the MXNet CI Dev account.
Expected result - creation of a directory of the form <commit-id>-jenkins-<job-name>-<build-id>
That directory should contain corresponding artifacts.

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@ChaiBapchya
Copy link
Contributor Author

@aaronmarkham
Copy link
Contributor

Where does the s3Upload function come from? This? https://github.com/jenkinsci/pipeline-aws-plugin
Also, have you put any docs together on the flow for this on the wiki or elsewhere?
Did you add the plugin to ci-dev and prod already?

@ChaiBapchya
Copy link
Contributor Author

Correct plugin.
Yes I tested it with dev.
Added it to prod.

Any recommendation where I should document this workflow?

@aaronmarkham
Copy link
Contributor

Correct plugin.
Yes I tested it with dev.
Added it to prod.

Any recommendation where I should document this workflow?

https://cwiki.apache.org/confluence/display/MXNET/Continuous+Integration

Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

How are you doing authentication? Are you using the instance profile or did you add credentials to the CI?

@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Oct 1, 2019

@aaronmarkham

Correct plugin.
Yes I tested it with dev.
Added it to prod.
Any recommendation where I should document this workflow?

https://cwiki.apache.org/confluence/display/MXNET/Continuous+Integration

Added the workflow here - https://cwiki.apache.org/confluence/display/MXNET/Tracking+Unit-tests

@marcoabreu I added the credentials to CI. Can you also review the cwiki. Thanks

@marcoabreu
Copy link
Contributor

Yeah we're not allowed to add AWS credentials to CI. Please remove them.

@marcoabreu marcoabreu dismissed their stale review October 1, 2019 18:47

Credentials weren't used but instance profiles

@marcoabreu
Copy link
Contributor

Okay, we clarified over Slack and Chai is in fact using instance profiles based on the jenkins-slave role paired with a policy that grants specific access to that particular bucket. That's exactly the way to go, so we're fine to move forward security-wise.

Still waiting for the exception swallow before approving.

Copy link
Contributor

@mseth10 mseth10 left a comment

Choose a reason for hiding this comment

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

LGTM

@marcoabreu marcoabreu merged commit 480b50c into apache:master Oct 2, 2019
@ChaiBapchya ChaiBapchya mentioned this pull request Oct 3, 2019
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants