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

Fixing weekly scan to run in proper controlled manner. #615

Merged
merged 14 commits into from Sep 19, 2018

Conversation

Projects
None yet
4 participants
@mohammedzee1000
Copy link
Member

mohammedzee1000 commented Sep 6, 2018

  • Removing pipelinetriggers from weekly scan
  • Adding secret to store openshift master url
  • Adding controller job which runs as an os cronjob.

Signed-off-by: Mohammed Zeeshan Ahmed mohammed.zee1000@gmail.com

@mohammedzee1000 mohammedzee1000 force-pushed the mohammedzee1000:openshift_weekly_scan_fix branch 3 times, most recently from 03cf69b to 4a1f4be Sep 6, 2018

@mohammedzee1000

This comment has been minimized.

Copy link
Member Author

mohammedzee1000 commented Sep 6, 2018

Successful run:

Creation:

$  oc process -p OS_MASTER_URL="https://os-master-1.example.com:8443"  -f configmaps/os-master-config.yaml | oc apply -f -

$  oc process -p JENKINS_SECRET_NAME=jenkins-token-ck7j8 -f weekly-scan/master-controller.yaml | oc apply -f -

If No weekly scans present:

$ docker logs 51f8d2bdab76
Logged into "https://os-master-1.lon1.centos.org:8443" as "system:serviceaccount:cccp:jenkins" using the token provided.
You have one project on this server: "cccp"
Using project "cccp".
No projects to trigger

If present:

$ docker logs 9cd913be76c1
Logged into "https://os-master-1.lon1.centos.org:8443" as "system:serviceaccount:cccp:jenkins" using the token provided.
You have one project on this server: "cccp"
Using project "cccp".
build "wscan-arrfab-nanoc-latest-1" started
....

@mohammedzee1000 mohammedzee1000 force-pushed the mohammedzee1000:openshift_weekly_scan_fix branch 2 times, most recently from 8545533 to 4fc671e Sep 6, 2018

@mohammedzee1000

This comment has been minimized.

Copy link
Member Author

mohammedzee1000 commented Sep 10, 2018

FYI you can use oc describe sa/jenkins and pick up token from there.

@bamachrn

This comment has been minimized.

Copy link
Collaborator

bamachrn commented Sep 11, 2018

Even though the CI shows as running here, it actually passed. As we have updated the status context, previous context is lost.

@dharmit
Copy link
Collaborator

dharmit left a comment

  • Since the confimap is made for weekly scan, can we please put it into weekly-scan directory and not create a separate one for it?
  • As far as I can see, we are adding templates. Why can't we have everything in one template? Or that's not the recommended way to do things in OpenShift/Kubernetes world?
Show resolved Hide resolved weekly-scan/master-controller.yaml Outdated
Show resolved Hide resolved weekly-scan/master-controller.yaml Outdated
Show resolved Hide resolved weekly-scan/master-controller.yaml Outdated
Show resolved Hide resolved weekly-scan/master-controller.yaml Outdated
command: ["/bin/sh", "/opt/entry/script"]
volumeMounts:
- name: entryscript
mountPath: /opt/entry

This comment has been minimized.

@dharmit

dharmit Sep 11, 2018

Collaborator

We're mounting this at /opt/entry and the name is entryscript. Are we sure that this won't cause issues with /opt/entry/script value of the command or did we mean to use /opt/entryscript in command ? Also, why not choose to keep this in /tmp instead ? /opt is generally for extra installation and packages.

- name: jenkins-secret
secret:
secretName: ${JENKINS_SECRET_NAME}
restartPolicy: OnFailure

This comment has been minimized.

@dharmit

dharmit Sep 11, 2018

Collaborator

What does this restartPolicy exactly do? If cron failed for some reason, it would restart it? What if there's some bug and we end up restarting it each time? Would that send weekly scan emails in a loop to the users?

secretName: ${JENKINS_SECRET_NAME}
restartPolicy: OnFailure
parameters:
- description: The amount of delay to give before triggering each weekly scan.

This comment has been minimized.

@dharmit

dharmit Sep 11, 2018

Collaborator

in seconds, milliseconds, ?

@mohammedzee1000

This comment has been minimized.

Copy link
Member Author

mohammedzee1000 commented Sep 11, 2018

@dharmit for why separate config map directory. It is because it is a common configmap, containing dns of openshift master, which has the potential of being used elsewhere in future. Hence i brought it outside

@dharmit

This comment has been minimized.

Copy link
Collaborator

dharmit commented Sep 11, 2018

@mohammedzee1000 with potential future use cases, we can't do things right now. If in future we do end up in a scenario you think we would end up in, we can change things like this then since it's just about doing mkdir; git mv. My concern is the directory structure of our repo. It's already having things everywhere.

@mohammedzee1000 mohammedzee1000 force-pushed the mohammedzee1000:openshift_weekly_scan_fix branch from 4fc671e to 214b19b Sep 11, 2018

@bamachrn bamachrn added the prod label Sep 12, 2018

@dharmit
Copy link
Collaborator

dharmit left a comment

Great stuff overall. But there are a few questions/concerns I would like to get addressed.

metadata:
name: weekly-scan-scheduler
spec:
schedule: "*/1 * * * *"

This comment has been minimized.

@dharmit

dharmit Sep 12, 2018

Collaborator

This will run the cron every minute. That's not what we want.

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 12, 2018

Author Member

Its not that is why its WIP, that will be final commit to move it to correct cron value. Its currently per minute to ease testing

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

is testing done ?

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

as in- if testing is done, we can update it to desired one.

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 17, 2018

Author Member

ok, updating

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 17, 2018

Author Member

yes, i have tested it and it works

- description: The delay in seconds to give before triggering each weekly scan.
displayName: Batch Trigger Delay
name: BATCH_DELAY
value: "300"

This comment has been minimized.

@dharmit

dharmit Sep 12, 2018

Collaborator

We can surely live with lower defaults but yeah this is OK till we move things to the prod for first time. Better safe than sorry.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

The complete build (including all stages takes around ~3 minutes) though!

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

Also we'll have more than 1 node.

OS_TOKEN=`cat /tmp/jenkins-secret/token`
oc login ${OS_MASTER} --insecure-skip-tls-verify=true --token=${OS_TOKEN};
for item in `oc get bc -o name --selector app=weekly-scan --selector type=pipeline`; do
GOT_ANY='1'

This comment has been minimized.

@dharmit

dharmit Sep 12, 2018

Collaborator

Keeping this inside for loop would cause GOT_ANY's value to be set to 1 every time. I think we want it to be 1 for checking things in the end. Can we instead have an if condition before triggering the for loop that sets value to 1. That way we can exit before the for loop. Example:

GOT_ANY='0'
OS_TOKEN=`cat /tmp/jenkins-secret/token`
oc login ${OS_MASTER} --insecure-skip-tls-verify=true --token=${OS_TOKEN};

if oc get bc -o name --selector app=weekly-scan --selector type=pipeline | wc -l > 1; then
  GOT_ANY=1
else
  echo "No projects to trigger"
  exit 0;
fi

for item in `oc get bc -o name --selector app=weekly-scan --selector type=pipeline`; do
  oc start-build ${item};
  sleep ${BATCH_DELAY}
done

This comment has been minimized.

@dharmit

dharmit Sep 12, 2018

Collaborator

Going a step further, we can do things without the extra variable as well:

OS_TOKEN=`cat /tmp/jenkins-secret/token`
oc login ${OS_MASTER} --insecure-skip-tls-verify=true --token=${OS_TOKEN};

if oc get bc -o name --selector app=weekly-scan --selector type=pipeline | wc -l ==0; then
  echo "No projects to trigger"
  exit 0;
fi

for item in `oc get bc -o name --selector app=weekly-scan --selector type=pipeline`; do
  oc start-build ${item};
  sleep ${BATCH_DELAY}
done

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 12, 2018

Author Member

Hmm, makes sense. Implementing

- name: jenkins-secret
secret:
secretName: ${JENKINS_SECRET_NAME}
restartPolicy: OnFailure

This comment has been minimized.

@dharmit

dharmit Sep 12, 2018

Collaborator

restartPolicy question is still open. Copying over here:

What does this restartPolicy exactly do? If cron failed for some reason, it would restart it? What if there's some bug and we end up restarting it each time? Would that send weekly scan emails in a loop to the users?

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 12, 2018

Author Member

Well, it could happen, although chances of this job failing are pretty slim, unless there is failure in the oc commands. Anycase, if you guys want it removed, i can.

@mohammedzee1000 mohammedzee1000 force-pushed the mohammedzee1000:openshift_weekly_scan_fix branch 3 times, most recently from e105437 to 8ef2ed5 Sep 12, 2018

@mohammedzee1000

This comment has been minimized.

Copy link
Member Author

mohammedzee1000 commented Sep 14, 2018

I dont get what test is failing here

data:
url: ${OS_MASTER_URL}
parameters:
- description: The url of openshift master, including https and port

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

*URL of OpenShift

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 17, 2018

Author Member

Updated

url: ${OS_MASTER_URL}
parameters:
- description: The url of openshift master, including https and port
displayName: Openshift Master URL

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

*OpenShift

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 17, 2018

Author Member

Updated

Show resolved Hide resolved weekly-scan/scheduler.yaml
metadata:
name: weekly-scan-scheduler
spec:
schedule: "*/1 * * * *"

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

is testing done ?

containers:
- name: weekly-scan-scheduler
image: ${JENKINS_SLAVE_IMAGE_NAME}
command: ["/bin/sh", "/tmp/entry/script"]

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

shouldn't this be entrypoint? The entrypoint set in slave image is jenkins specific, if you provide this as command, it will given as arg to entrypoint set.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

I think this won't work with command instruction - did you test this ?

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 14, 2018

Author Member

Yes it is tested. Infact, entrypoint will fail here.

This is because the entrypoint script handles some permission stuff to run this container on Openshift

- description: The delay in seconds to give before triggering each weekly scan.
displayName: Batch Trigger Delay
name: BATCH_DELAY
value: "300"

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

Also we'll have more than 1 node.

name: BATCH_DELAY
value: "300"
required: true
- description: The jenkins slave image name.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

*Jenkins

required: true
- description: The jenkins slave image name.
displayName: Jenkins Slave Image
name: JENKINS_SLAVE_IMAGE_NAME

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

I've used following for this image in email notifications - We should unify the parameter name.

- description: ccp-openshift-slave container image name (with registry name)
  displayName: ccp-openshift-slave container image
  name: CCP_OPENSHIFT_SLAVE_IMAGE
  required: true
  value: registry.centos.org/pipeline-images/ccp-openshift-slave:latest

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 17, 2018

Author Member

updated

name: JENKINS_SLAVE_IMAGE_NAME
value: registry.centos.org/pipeline-images/ccp-openshift-slave
required: true
- description: The name of jenkins token to mount.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

*Jenkins

value: registry.centos.org/pipeline-images/ccp-openshift-slave
required: true
- description: The name of jenkins token to mount.
displayName: Jenkins Token Secret Name

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

*token

This comment has been minimized.

@navidshaikh

navidshaikh Sep 14, 2018

Collaborator

*name

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 17, 2018

Author Member

Updated

@mohammedzee1000 mohammedzee1000 changed the title [WIP] Fixing weekly scan to run in proper controlled manner. Fixing weekly scan to run in proper controlled manner. Sep 17, 2018

@navidshaikh
Copy link
Collaborator

navidshaikh left a comment

LGTM

@navidshaikh

This comment has been minimized.

Copy link
Collaborator

navidshaikh commented Sep 17, 2018

@mohammedzee1000 : We'll need to update the deployment doc addressing how/when to create these objects in weekly-scan/ folder and their order as well.

@mohammedzee1000

This comment has been minimized.

Copy link
Member Author

mohammedzee1000 commented Sep 17, 2018

Yes, that will be needed

@mohammedzee1000 mohammedzee1000 force-pushed the mohammedzee1000:openshift_weekly_scan_fix branch from d6bdd9c to c1cd28a Sep 17, 2018

README.md Outdated

Now lets setup weekly scan triggering mechanism. Note the pipelines will be created by seed job, but without triggers.

First, setup a configmap that contains URL of openshift master as below. Note, ports if any, must be included.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

I keep asking for these changes -
The readme has same word written differently at other places, we should stick to unify represting all these product's names.

Please update the line
*ConfigMap
*OpenShift

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 17, 2018

Author Member

Updated

This comment has been minimized.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

Nope. I see still see openshift and configmap.

README.md Outdated
$ oc process -p OS_MASTER_URL="https://os-master.example.com:8443" -f weekly-scan/os-master-config.yaml | oc apply -f -
```

Now, we need the jenkins token to be made available. To do this, do the following

This comment has been minimized.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

*Jenkins

This comment has been minimized.

@mohammedzee1000

mohammedzee1000 Sep 17, 2018

Author Member

Updated

This comment has been minimized.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

nope, I see jenkins there.

@mohammedzee1000 mohammedzee1000 force-pushed the mohammedzee1000:openshift_weekly_scan_fix branch 2 times, most recently from 2885633 to ffddf7d Sep 17, 2018

@navidshaikh
Copy link
Collaborator

navidshaikh left a comment

Its been 3 iterations to ask for same review feedback and fixes, specifically for English statements in the PR.

I'd still want to request changes, however since we need to move faster for prod deployment and testing, I won't block this PR to go in.

We have reviews from other PRs about streamlining the parameter definitions in templates, I am hoping to have the fixes in that work.

name: BATCH_DELAY
value: "180"
required: true
- description: The jenkins slave image name.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 18, 2018

Collaborator

We have inconsistencies.

If a reviewer has asked to update few strings, the author should look up for matching issue in whole file than the one incident pointed out. It was a line just above where again jenkins is used.

value: "180"
required: true
- description: The jenkins slave image name.
displayName: Jenkins Slave Image

This comment has been minimized.

@navidshaikh

navidshaikh Sep 18, 2018

Collaborator

Now mixed casing used here, else where we are just using first letter as capital while in this we are using J S and I.
*Jenkins slave image was just fine too.

@navidshaikh

This comment has been minimized.

Copy link
Collaborator

navidshaikh commented Sep 18, 2018

I am dismissing my review and we can proceed further with @dharmit LGTM on it. Dharmit - once the CI is green, please go ahead and merge it.

@bamachrn

This comment has been minimized.

Copy link
Collaborator

bamachrn commented Sep 19, 2018

#dotests

3 similar comments
@navidshaikh

This comment has been minimized.

Copy link
Collaborator

navidshaikh commented Sep 19, 2018

#dotests

@navidshaikh

This comment has been minimized.

Copy link
Collaborator

navidshaikh commented Sep 19, 2018

#dotests

@bamachrn

This comment has been minimized.

Copy link
Collaborator

bamachrn commented Sep 19, 2018

#dotests

@mohammedzee1000 mohammedzee1000 force-pushed the mohammedzee1000:openshift_weekly_scan_fix branch from ffddf7d to 54804dc Sep 19, 2018

@bamachrn

This comment has been minimized.

Copy link
Collaborator

bamachrn commented Sep 19, 2018

#dotests-unittests

@dharmit dharmit merged commit 86640b1 into CentOS:openshift Sep 19, 2018

2 checks passed

centos-ci functional tests centos-ci functional tests succeeded
Details
centos-ci unit tests centos-ci unit tests succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.