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

Processes the index projects in batches #612

Merged
merged 10 commits into from Sep 17, 2018

Conversation

Projects
None yet
3 participants
@navidshaikh
Collaborator

navidshaikh commented Sep 6, 2018

This changeset updates index reader to process index-projects
in defined (default=5) length of batches. Index reader, splits
the index projects in batches and processes each batch one by one.

In between processing batches of projects, it checks if there are
any running builds. If it finds a running build, it waits for given
poll_cyle (default=120 seconds) and checks again. As it finds, openshift
idle (not processing any builds ATM), it queues the batch and waits for
its completion as above.

The weekly scan projects are processed at the end after processing build
jobs completely. Every weekly scan job is processed with 5 seconds delay
in between.

Sample output first run of the index

[pipeline-service] Running shell script
+ python ccp/index_reader.py /tmp/container-index/index.d 172.29.33.53:5000 cccp nshaikh@redhat.com smtp://mail.centos.org
Number of projects in index 9
Number of projects in OpenShift 0
Number of projects to be updated/created: 9
Processing projects batch: ['test-anomaly-latest', 'test-python-release', 'dharmit-freegeoip-latest', 'dharmit-nsq-latest', 'mattermost-mattermost-github-integration-92394f2']
buildconfig.build.openshift.io "test-anomaly-latest" created

buildconfig.build.openshift.io "test-python-release" created

buildconfig.build.openshift.io "dharmit-freegeoip-latest" created

buildconfig.build.openshift.io "dharmit-nsq-latest" created

buildconfig.build.openshift.io "mattermost-mattermost-github-integration-92394f2" created

Processing projects batch: ['arrfab-nanoc-latest', 'openshift-origin-base-latest', 'openshift-origin-source-latest', 'centos-centos-latest']
buildconfig.build.openshift.io "arrfab-nanoc-latest" created

buildconfig.build.openshift.io "openshift-origin-base-latest" created

buildconfig.build.openshift.io "openshift-origin-source-latest" created

buildconfig.build.openshift.io "centos-centos-latest" created

Processing weekly scan projects..
buildconfig.build.openshift.io "wscan-test-anomaly-latest" created

buildconfig.build.openshift.io "wscan-test-python-release" created

buildconfig.build.openshift.io "wscan-dharmit-freegeoip-latest" created

buildconfig.build.openshift.io "wscan-dharmit-nsq-latest" created

buildconfig.build.openshift.io "wscan-mattermost-mattermost-github-integration-92394f2" created

buildconfig.build.openshift.io "wscan-arrfab-nanoc-latest" created

buildconfig.build.openshift.io "wscan-openshift-origin-base-latest" created

buildconfig.build.openshift.io "wscan-openshift-origin-source-latest" created

buildconfig.build.openshift.io "wscan-centos-centos-latest" created
@navidshaikh

This comment has been minimized.

Show comment
Hide comment
@navidshaikh

navidshaikh Sep 6, 2018

Collaborator

This PR is rebased on #617, which needs to go first.

Collaborator

navidshaikh commented Sep 6, 2018

This PR is rebased on #617, which needs to go first.

@bamachrn

LGTM, otherwise

"""
pass
print (msg)
sys.stdout.flush()

This comment has been minimized.

@bamachrn

bamachrn Sep 11, 2018

Collaborator

we added the stdout flush to make sure jenkins is not holding them back from printing right?

@bamachrn

bamachrn Sep 11, 2018

Collaborator

we added the stdout flush to make sure jenkins is not holding them back from printing right?

This comment has been minimized.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

That's correct.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

That's correct.

output = output.strip().split(' ')
output = [each for each in output
if not each.startswith(tuple(filter_builds))
and each]

This comment has been minimized.

@bamachrn

bamachrn Sep 11, 2018

Collaborator

OK, please add the filter_builds to the function documentation as well

@bamachrn

bamachrn Sep 11, 2018

Collaborator

OK, please add the filter_builds to the function documentation as well

This comment has been minimized.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

Ack!

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

Ack!

Show outdated Hide outdated ccp/index_reader.py
Show outdated Hide outdated ccp/index_reader.py

@bamachrn bamachrn added the prod label Sep 12, 2018

@bamachrn bamachrn self-assigned this Sep 12, 2018

navidshaikh added some commits Sep 4, 2018

Processes the index projects in batches
 This changeset updates index reader to process index-projects
 in defined (default=5) length of batches. Index reader, splits
 the index projects in batches and processes each batch one by one.

 In between processing batches of projects, it checks if there are
 any running builds. If it finds a running build, it waits for given
 poll_cyle (default=120 seconds) and checks again. As it finds, openshift
 idle (not processing any builds ATM), it queues the batch and waits for
 its completion as above.

 The weekly scan projects are processed at the end after processing build
 jobs completely. Every weekly scan job is processed with 5 seconds delay
 in between.
Improves the wait cycle workflow in between processing batches
 Also adds new lines characters for improved readability
Separates the custom exception into a ccp/exceptions.py
 Also adds PYTHONPATH before invoking index_reader, as
 index_reader.py is now referenceing the other modules.
Parameterizes batch size, polling interval and outstanding builds cap
  Batch processing can now be configured for
  - batch size - number of projects to be processed in each batch
  - polling interval - polling interval to check the outstanding builds
  - outstanding builds cap - Number of maximum outstanding builds, on which next batch can be scheduled

  Added these parameters in seed-job/template.yaml with default 5, 30, 3 respectively.
Continues processing rest YAML files encountered error processing one
   If there is an error processing any of the YAML file in index,
   log the error and continue processing rest of the YAML entries.

   Also updates the docstrings.
@navidshaikh

This comment has been minimized.

Show comment
Hide comment
@navidshaikh

navidshaikh Sep 12, 2018

Collaborator

Rebased and fixed the review comments.

Collaborator

navidshaikh commented Sep 12, 2018

Rebased and fixed the review comments.

@dharmit

Looks great overall. Raised a few points that might be worth discussing with the team. Few changes requested mainly for docstrings.

Show outdated Hide outdated seed-job/buildtemplate.yaml
Show outdated Hide outdated seed-job/buildtemplate.yaml
Show outdated Hide outdated seed-job/buildtemplate.yaml
@@ -63,7 +63,7 @@ objects:
}
stage('Parse index') {
dir("${PIPELINE_REPO_DIR}") {
sh "python ccp/index_reader.py ${CONTAINER_INDEX_DIR}/index.d ${REGISTRY_URL} ${NAMESPACE} ${FROM_ADDRESS} ${SMTP_SERVER}"
sh "PYTHONPATH=${PIPELINE_REPO_DIR} python ccp/index_reader.py ${CONTAINER_INDEX_DIR}/index.d ${REGISTRY_URL} ${NAMESPACE} ${FROM_ADDRESS} ${SMTP_SERVER} ${BATCH_SIZE} ${BATCH_POLLING_INTERVAL} ${BATCH_OUTSTANDING_BUILDS_CAP}"

This comment has been minimized.

@dharmit

dharmit Sep 16, 2018

Contributor

This is getting longer as we add more parameters to be passed to index reader. Should we use named arguments here and use argparse module in index_reader.py to make things more clear? A minor error could lead to unexpected results. I understand this piece is buried quite deep into the code and probably would never be stumbled upon by someone trying to deploy the service. But for someone trying to develop, it could be helpful.

Thoughts @navidshaikh @bamachrn @mohammedzee1000

@dharmit

dharmit Sep 16, 2018

Contributor

This is getting longer as we add more parameters to be passed to index reader. Should we use named arguments here and use argparse module in index_reader.py to make things more clear? A minor error could lead to unexpected results. I understand this piece is buried quite deep into the code and probably would never be stumbled upon by someone trying to deploy the service. But for someone trying to develop, it could be helpful.

Thoughts @navidshaikh @bamachrn @mohammedzee1000

This comment has been minimized.

@dharmit

dharmit Sep 16, 2018

Contributor

I don't mean to do it right away but if it makes sense, we must add a card for this

@dharmit

dharmit Sep 16, 2018

Contributor

I don't mean to do it right away but if it makes sense, we must add a card for this

This comment has been minimized.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

done.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

done.

if __name__ == "__main__":
if len(sys.argv) != 6:
print ("Incomplete set of input variables, please refer README.")
if len(sys.argv) != 9:

This comment has been minimized.

@dharmit

dharmit Sep 16, 2018

Contributor

Same concern as that in the command where we do PYTHONPATH=${PIPELINE_REPO_DIR}...

@dharmit

dharmit Sep 16, 2018

Contributor

Same concern as that in the command where we do PYTHONPATH=${PIPELINE_REPO_DIR}...

@@ -415,14 +467,15 @@ def find_stale_jobs(self, oc_projects, index_projects):
return list(set(oc_projects) - set(index_projects))
def run(self):
def run(self, batch_size, polling_interval,
batch_outstanding_builds_cap):

This comment has been minimized.

@dharmit

dharmit Sep 16, 2018

Contributor

I think we can add more documentation here about the scope of run method. It does quite a few things like:

  • reading the projects in the container index
  • removing stale projects
  • adding projects and weekly scan jobs

The one-line description is perfect. But adding detail about what orchestrate means would be helpful for future reference.

@dharmit

dharmit Sep 16, 2018

Contributor

I think we can add more documentation here about the scope of run method. It does quite a few things like:

  • reading the projects in the container index
  • removing stale projects
  • adding projects and weekly scan jobs

The one-line description is perfect. But adding detail about what orchestrate means would be helpful for future reference.

Watch the outstanding builds at $polling_interval and
schedule next batch if current outstanding jobs are
less than or equal to $batch_outstanding_builds_cap.
"""

This comment has been minimized.

@dharmit

dharmit Sep 16, 2018

Contributor

👍 for clear comments about the scope of function.

@dharmit

dharmit Sep 16, 2018

Contributor

👍 for clear comments about the scope of function.

Show outdated Hide outdated ccp/index_reader.py
outstanding_builds = self.bc_manager.list_builds_except(
status=["Complete", "Failed"],
filter_builds=self.infra_projects)

This comment has been minimized.

@dharmit

dharmit Sep 16, 2018

Contributor

IIUC, we're executing a shell command to get output here. run_cmd and list_builds_except don't seem to have exception handling. So, if fetching outstanding builds fails due to some issue (like failure in executing oc command), seed-job would fail. Correct me if I'm wrong.

If I'm correct, I think this part is deep into the execution of our service. We could give a few retries to the scenario where oc fails to get our desired task done and fail the seed-job appropriate message if it fails even after the retries.

Thoughts @navidshaikh @bamachrn @mohammedzee1000 ?

@dharmit

dharmit Sep 16, 2018

Contributor

IIUC, we're executing a shell command to get output here. run_cmd and list_builds_except don't seem to have exception handling. So, if fetching outstanding builds fails due to some issue (like failure in executing oc command), seed-job would fail. Correct me if I'm wrong.

If I'm correct, I think this part is deep into the execution of our service. We could give a few retries to the scenario where oc fails to get our desired task done and fail the seed-job appropriate message if it fails even after the retries.

Thoughts @navidshaikh @bamachrn @mohammedzee1000 ?

This comment has been minimized.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

I think it makes sense - We can keep a retry cycle of 30 seconds with timeout limit set to 5 minutes ? And fail it is 5 minutes (10 retries) are lapsed ?

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

I think it makes sense - We can keep a retry cycle of 30 seconds with timeout limit set to 5 minutes ? And fail it is 5 minutes (10 retries) are lapsed ?

This comment has been minimized.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

It makes sense. It applies to all the operations performed via oc. Adding a retry decorator which can be re-used and have it applied to al oc related operations.

@navidshaikh

navidshaikh Sep 17, 2018

Collaborator

It makes sense. It applies to all the operations performed via oc. Adding a retry decorator which can be re-used and have it applied to al oc related operations.

navidshaikh added some commits Sep 17, 2018

Adds a retry decorator for oc related operations
  Added a retry decorator with retries count, delay count in seconds
  and backoff count in seconds.

  Added this decorator to all the oc related operations.
@bamachrn

LGTM

@bamachrn bamachrn merged commit 185bd7a into CentOS:openshift Sep 17, 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