Check if AWS_SHARED_CREDENTIALS_FILE is set #30

hongkailiu · 2019-07-23T20:47:58Z

follow-up of openshift/release#4489

Follow-up of openshift/release#4489

cmd/ipi-deprovision/ipi-deprovision.sh

stevekuznetsov

/lgtm
/approve

stevekuznetsov · 2019-07-23T21:40:58Z

/lgtm

hongkailiu · 2019-07-23T21:44:27Z

/retest

stevekuznetsov · 2019-07-23T21:53:52Z

/refresh
/retest

stevekuznetsov · 2019-07-23T21:55:25Z

/close

hongkailiu · 2019-07-24T00:17:41Z

/hold debugging

hongkailiu · 2019-07-24T00:35:41Z

/hold cancel

stevekuznetsov · 2019-07-24T00:54:28Z

cmd/ipi-deprovision/ipi-deprovision.sh

-declare -a regions=("us-east-1" "us-east-2" "us-west-1")
+my_date=$(TZ=":Africa/Abidjan" date '+%Y-%m-%d')
+
+if [[ -n "${IPI_DEPROVISION_DATE-}" ]]; then


Hmm sorry -- we will want this to be relative to the time that is run, right? So the config option would be how old --

$ date --date='1 hour ago' Tue Jul 23 16:54:10 PDT 2019

stevekuznetsov · 2019-07-24T02:22:45Z

cmd/ipi-deprovision/ipi-deprovision.sh

@@ -1,17 +1,44 @@
 #!/bin/sh


So I don't think we want to run POSIX shell. If we have bash, let's use it.

Done. CP/Paste error.

cmd/ipi-deprovision/ipi-deprovision.sh

stevekuznetsov · 2019-07-24T02:24:21Z

cmd/ipi-deprovision/ipi-deprovision.sh

+set -o nounset
+set -o pipefail
+
+if [ -z "${AWS_SHARED_CREDENTIALS_FILE}" ]; then echo "ENV VAR AWS_SHARED_CREDENTIALS_FILE is not set"; exit 1; fi


nit: prefer

if [[ -z "${AWS_SHARED_CREDENTIALS_FILE:-}" ]]; then echo "Required variable \$AWS_SHARED_CREDENTIALS_FILE unset" exit 1 fi

Also, can't we just omit this? The AWS library will fail when no credentials are found, and this script could be used locally with the credentials in $HOME

stevekuznetsov · 2019-07-24T02:24:54Z

cmd/ipi-deprovision/ipi-deprovision.sh

+
+if [ -z "${AWS_SHARED_CREDENTIALS_FILE}" ]; then echo "ENV VAR AWS_SHARED_CREDENTIALS_FILE is not set"; exit 1; fi
+
 ### expirationDate is always with timezone +00:00, eg, "2019-07-23T22:35+0000"


This comment is out of place

Improved a bit.

stevekuznetsov · 2019-07-24T02:25:20Z

cmd/ipi-deprovision/ipi-deprovision.sh

-declare -a regions=("us-east-1" "us-east-2" "us-west-1")
+my_date=$(TZ=":Africa/Abidjan" date '+%Y-%m-%d')
+
+if [[ -n "${IPI_DEPROVISION_DATE_ARG-}" ]]; then


We should validate that this is set, as without it, the script is not useful.

nit: that's a lot of names in the env var! we could just use something like ${CLUSTER_TTL}

rename: Done.
validation: not sure how to validate it. Could do it as a follow-up i think.

stevekuznetsov · 2019-07-24T02:27:00Z

cmd/ipi-deprovision/ipi-deprovision.sh

+
+if [[ -n "${IPI_DEPROVISION_DATE_ARG-}" ]]; then
+  ### eg, export IPI_DEPROVISION_DATE_ARG="22 hours ago"
+  echo "take env. var.: IPI_DEPROVISION_DATE_ARG: ${IPI_DEPROVISION_DATE_ARG}"


nit: maybe format like

echo "Searching for clusters with a TTL of ${IPI_DEPROVISION_DATE_ARG}"

or

echo "Searching for clusters older than ${my_date}

stevekuznetsov · 2019-07-24T02:27:56Z

cmd/ipi-deprovision/ipi-deprovision.sh

+
+echo "my_date: ${my_date}"
+
+### James: CI only runs in us-east-1.


we don't need to keep this comment around, I don't think

stevekuznetsov · 2019-07-24T02:29:05Z

cmd/ipi-deprovision/ipi-deprovision.sh

+declare -a regions=("us-east-1")
+### for more than one region:
+# declare -a regions=("us-east-1" "us-east-2" "us-west-1")
+if [[ -n "${IPI_DEPROVISION_REGIONS-}" ]]; then


Do we need this? We can add it later if we do

stevekuznetsov · 2019-07-24T02:29:23Z

cmd/ipi-deprovision/ipi-deprovision.sh

+if [[ -n "${IPI_DEPROVISION_REGIONS-}" ]]; then
+  ### eg, export IPI_DEPROVISION_REGIONS="us-east-1;us-west-1"
+  echo "take regions from env. var.: IPI_DEPROVISION_REGIONS: ${IPI_DEPROVISION_REGIONS}"
+  IFS=';' read -r -a regions <<< "$IPI_DEPROVISION_REGIONS"


FYI read -a and <<< here-string are not defined in POSIX shell :)

Good to know.

stevekuznetsov · 2019-07-24T02:29:35Z

cmd/ipi-deprovision/ipi-deprovision.sh

+  IFS=';' read -r -a regions <<< "$IPI_DEPROVISION_REGIONS"
+fi
+
+echo "regions: ${regions}"


likely do not need this debugger

stevekuznetsov

Everyone gets their first real shell review from me someday :)

stevekuznetsov · 2019-07-24T02:30:01Z

cmd/ipi-deprovision/ipi-deprovision.sh

 handle_cluster () {
  local cluster_name;
  cluster_name=$1
  echo "handling cluster: ${cluster_name} ..."


might be nice to see the date it expired at, as well

/hold ... I want to give this a try.

/hold cancel
There should be a way to do it with one search and pass 2 params into the function.
Let us iterate.

stevekuznetsov · 2019-07-24T02:31:22Z

cmd/ipi-deprovision/ipi-deprovision.sh

 do
  echo "doing region ${r} ..."
-  aws ec2 describe-vpcs --output json --region "${r}" | jq --arg date "${MY_DATE}" -r -S '.Vpcs[] | select (.Tags[]? | (.Key == "expirationDate" and .Value < $date)) | .Tags[] | select (.Value == "owned") | .Key' | while read line; do handle_cluster ${line}; done
+  aws ec2 describe-vpcs --output json --region "${r}" | jq --arg date "${my_date}" -r -S '.Vpcs[] | select (.Tags[]? | (.Key == "expirationDate" and .Value < $date)) | .Tags[] | select (.Value == "owned") | .Key' | while read line; do handle_cluster ${line}; done


nit: piping into while read line is a bit esoteric, we could:

for cluster in $( aws ec2 describe... ); do handle_cluster "${cluster}" done

(and with that formatting we hardly need the handle_cluster function since inlining it is simple

stevekuznetsov · 2019-07-24T03:54:04Z

cmd/ipi-deprovision/ipi-deprovision.sh

+### which is always with timezone +00:00, eg, "2019-07-23T22:35+0000"
+cluster_age_cutoff=$(TZ=":Africa/Abidjan" date '+%Y-%m-%d')
+
+if [[ -n "${CLUSTER_TTL-}" ]]; then


So if this is not set, we will remove all clusters with an expiration date older than now? This is a dangerous default -- we should enforce that this parameter is passed and not use a default

stevekuznetsov · 2019-07-24T03:56:22Z

cmd/ipi-deprovision/ipi-deprovision.sh

  local cluster_name;
  cluster_name=$1
-  echo "handling cluster: ${cluster_name} ..."
+  echo "handling cluster: ${cluster_name} with expirationDate: $(aws ec2 describe-vpcs --output json --region "us-east-1" --filters "Name=tag:${cluster_name},Values=owned" | jq -r '.Vpcs[].Tags[] | select (.Key=="expirationDate") | .Value')"


nit: this will make runtime much slower as we now make O(N) aws calls (rather than one list) and be also more costly on API use -- we can save the output of aws describe-vpcs --output json into a file and post-process that or use jq in the first call to select the cluster name and expiration date, either way would work.

Done.
I like the 2nd way more but i can implement the 1st one for the moment.

hongkailiu · 2019-07-24T13:21:09Z

/hold WIP

hongkailiu · 2019-07-24T13:50:39Z

/hold cancel

stevekuznetsov

/lgtm
/approve

openshift-ci-robot · 2019-07-24T14:56:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongkailiu, stevekuznetsov

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:

~~OWNERS~~ [hongkailiu,stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

# This is the 1st commit message: force all disruption aggregation to skipped status while we re-gather unbugged data # This is the commit message openshift#2: Fix lint issue. # This is the commit message openshift#3: sanitize: keep jobs from the same PR on the same cluster # This is the commit message openshift#4: Support runIfChanged and skipIfOnlyChanged for postsubmit Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#5: Update UT Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#6: fix integration test Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#7: [DPTP-2635]cluster-display: add endpont for clusters # This is the commit message openshift#8: prowgen: stop tolerating changes to `optional` fields ...except for images jobs for which do not have syntax in ci-operator config Once we pull all manual changes to ci-operator configs, we can stop tolerating these changes. One step closer to fully generated Prowjob configuration. # This is the commit message openshift#9: Honor semver when comparing clone target/source # This is the commit message openshift#10: Write a README for autoconfigbrancher # This is the commit message openshift#11: autoconfigbrancher: simplify a conditional # This is the commit message openshift#12: autoconfigbrancher: use a permanent link in README # This is the commit message openshift#13: Rename mis-spelled variable, add short for cobra help, add comments # This is the commit message openshift#14: add more comments re: GCS # This is the commit message openshift#15: Note two tables created in memory using a SELECT # This is the commit message openshift#16: fix typo # This is the commit message openshift#17: cast to a type so we can see more info on dryrun mode # This is the commit message openshift#18: Share ReleaseController's config # This is the commit message openshift#19: Add aggregating job results to testgrid (openshift#2576) * Add aggregating job results to testgrid * Add aggregating job results to testgrid Aggregating jobs are added to blocking dashboard of the corresponding release. Release is determined by the aggregated job prow config. * Add test cases * Add nil pointer check # This is the commit message openshift#20: Updating auto-include logic for OpenStack jobs Adding the OpenStack jobs which recently moved from release to shiftstack directory. This will add them to testgrid automatically. # This is the commit message openshift#21: [DPTP-2660]payload-testing-prow-plugin: format errors # This is the commit message openshift#22: handle missing history for aggregated disruption # This is the commit message openshift#23: Add create-tables subcommand to create Jobs tab and debugging tips to README # This is the commit message openshift#24: create JobRuns table # This is the commit message openshift#25: Rename vars to be more generic for table creation # This is the commit message openshift#26: add in gofmt fixes for lint # This is the commit message openshift#27: comment about Jobs being initially primed # This is the commit message openshift#28: comment compile time checks in case go beginners have never seen it # This is the commit message openshift#29: prpqr: add a PR deploy makefile target # This is the commit message openshift#30: prpqr ui: use centos stream instead of 8 from docker # This is the commit message openshift#31: Refuse /payload command on PRs to repositories that do not contribute to OCP # This is the commit message openshift#32: allow correcting history without changing test name when we get criteria incorrect # This is the commit message openshift#33: switch the check for aggregation passes to use the SQL views

Check if AWS_SHARED_CREDENTIALS_FILE is set

93f9880

Follow-up of openshift/release#4489

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 23, 2019

openshift-ci-robot requested review from droslean and smarterclayton July 23, 2019 20:48

hongkailiu changed the title ~~Check if AWS_SHARED_CREDENTIALS_FILE is set~~ Set AWS_SHARED_CREDENTIALS_FILE for aws cli Jul 23, 2019

stevekuznetsov reviewed Jul 23, 2019

View reviewed changes

cmd/ipi-deprovision/ipi-deprovision.sh Outdated Show resolved Hide resolved

hongkailiu changed the title ~~Set AWS_SHARED_CREDENTIALS_FILE for aws cli~~ Check if AWS_SHARED_CREDENTIALS_FILE is set Jul 23, 2019

stevekuznetsov reviewed Jul 23, 2019

View reviewed changes

Add comments on ipi-deprovision.sh

706c647

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 24, 2019

stevekuznetsov reviewed Jul 24, 2019

View reviewed changes

Take params of ipi-deprovision.sh from env. var.

9dbcc5e

stevekuznetsov reviewed Jul 24, 2019

View reviewed changes

openshift-ci-robot assigned stevekuznetsov Jul 24, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2019

openshift-merge-robot merged commit 366a2fd into openshift:master Jul 24, 2019


		if [ -z "${AWS_SHARED_CREDENTIALS_FILE}" ]; then echo "ENV VAR AWS_SHARED_CREDENTIALS_FILE is not set"; exit 1; fi

		### expirationDate is always with timezone +00:00, eg, "2019-07-23T22:35+0000"


		echo "my_date: ${my_date}"

		### James: CI only runs in us-east-1.

Check if AWS_SHARED_CREDENTIALS_FILE is set #30

Check if AWS_SHARED_CREDENTIALS_FILE is set #30

Uh oh!

Conversation

hongkailiu commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

stevekuznetsov left a comment

Choose a reason for hiding this comment

Uh oh!

stevekuznetsov commented Jul 23, 2019

Uh oh!

hongkailiu commented Jul 23, 2019

Uh oh!

stevekuznetsov commented Jul 23, 2019

Uh oh!

stevekuznetsov commented Jul 23, 2019

Uh oh!

hongkailiu commented Jul 24, 2019

Uh oh!

hongkailiu commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevekuznetsov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hongkailiu commented Jul 23, 2019 •

edited

Loading