From cb2ea1a27bb1f9f49f20b0cfcf48206ff2a33b68 Mon Sep 17 00:00:00 2001 From: Chris Evich Date: Wed, 31 Jul 2019 09:01:49 -0400 Subject: Cirrus: Fix re-run of release task into no-op. This task depends upon other tasks caching their binaries. If for whatever reason the `release` task is re-run and/or is out-of-order with it's dependents, the state of cache will be undefined. Previously this would result in an error, and failing of the release task. This commit alters this behavior to issue a warning instead. Signed-off-by: Chris Evich --- contrib/cirrus/cache_release_archive.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/cirrus/cache_release_archive.sh b/contrib/cirrus/cache_release_archive.sh index 639bc9801..2365f7593 100755 --- a/contrib/cirrus/cache_release_archive.sh +++ b/contrib/cirrus/cache_release_archive.sh @@ -93,7 +93,7 @@ elif [[ "$(basename $0)" == "uncache_release_archives.sh" ]] then req_env_var CIRRUS_BUILD_ID CI_NODE_TOTAL GCPJSON GCPNAME GCPROJECT [[ "${CI_NODE_INDEX}" -eq "$[CI_NODE_TOTAL-1]" ]] || \ - die 8 "The release task must be executed last to guarantee archive cache is complete" + die 0 "WARNING: This task depends on cache data from other tasks, otherwise it is a no-op." if [[ -n "$CIRRUS_PR" ]] then -- cgit v1.2.3-54-g00ecf From 3e3afb942afc7f082d5e1e68f6c50dc677d19e3c Mon Sep 17 00:00:00 2001 From: Chris Evich Date: Wed, 31 Jul 2019 09:48:14 -0400 Subject: Cirrus: Fix release dependencies The release-task ***must*** always execute last, in order to guarantee a consistent cache of release archives from dependent tasks. It accomplishes this by verifying it's task-number matches one-less than the total number of tasks. Previous to this commit, a YAML anchor/alias was used to avoid duplication of the dependency list between 'success' and 'release' However, it's been observed that this opens the possibility for 'release' and 'success' tasks to race when running on a PR. Because YAML anchor/aliases cannot be used to modify lists, duplication is required to make 'release' actually depend upon 'success'. This duplication will introduce an additional maintenance burden. Though when adding a new task, it's already very easy to forget to update the 'depends_on' list. Assist both cases by the addition unit-tests to verify ``.cirrus.yml`` dependency contents and structure. Signed-off-by: Chris Evich --- .cirrus.yml | 20 +++++++++-- Makefile | 1 + contrib/cirrus/cirrus_yaml_test.py | 68 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 2 deletions(-) create mode 100755 contrib/cirrus/cirrus_yaml_test.py diff --git a/.cirrus.yml b/.cirrus.yml index faa0a531c..8389c638d 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -504,13 +504,14 @@ success_task: only_if: $CIRRUS_BRANCH != $DEST_BRANCH # ignores any dependent task conditions, include everything except 'release' - depends_on: &alltasks + depends_on: - "gating" - "vendor" - "varlink_api" - "build_each_commit" - "build_without_cgo" - "meta" + - "image_prune" - "testing" - "special_testing_rootless" - "special_testing_in_podman" @@ -540,7 +541,22 @@ release_task: # allow_failures: $CI == "true" # skip_notifications: $CI == "true" - depends_on: *alltasks + # Must include everything (YAML anchor/alias cannot be used here) + depends_on: + - "gating" + - "vendor" + - "varlink_api" + - "build_each_commit" + - "build_without_cgo" + - "meta" + - "image_prune" + - "testing" + - "special_testing_rootless" + - "special_testing_in_podman" + - "special_testing_cross" + - "test_build_cache_images" + - "verify_test_built_images" + - "success" gce_instance: image_name: "${IMAGE_BUILDER_CACHE_IMAGE_NAME}" diff --git a/Makefile b/Makefile index 476e4f054..f8eff61ba 100644 --- a/Makefile +++ b/Makefile @@ -232,6 +232,7 @@ localunit: test/goecho/goecho varlink_generate --succinct $(MAKE) -C contrib/cirrus/packer test ./contrib/cirrus/lib.sh.t + ./contrib/cirrus/cirrus_yaml_test.py ginkgo: ginkgo -v -tags "$(BUILDTAGS)" $(GINKGOTIMEOUT) -cover -flakeAttempts 3 -progress -trace -noColor -nodes 3 -debug test/e2e/. diff --git a/contrib/cirrus/cirrus_yaml_test.py b/contrib/cirrus/cirrus_yaml_test.py new file mode 100755 index 000000000..c8faee65f --- /dev/null +++ b/contrib/cirrus/cirrus_yaml_test.py @@ -0,0 +1,68 @@ +#!/usr/bin/env python3 + +""" +Verify contents of .cirrus.yml meet specific expectations +""" + +import sys +import os +import unittest +import yaml + +# Assumes directory structure of this file relative to repo. +SCRIPT_DIRPATH = os.path.dirname(os.path.realpath(__file__)) +REPO_ROOT = os.path.realpath(os.path.join(SCRIPT_DIRPATH, '../', '../')) + + +class TestCaseBase(unittest.TestCase): + + CIRRUS_YAML = None + + def setUp(self): + with open(os.path.join(REPO_ROOT, '.cirrus.yml')) as cirrus_yaml: + self.CIRRUS_YAML = yaml.safe_load(cirrus_yaml.read()) + + +class TestDependsOn(TestCaseBase): + + ALL_TASK_NAMES = None + SUCCESS_RELEASE = set(['success', 'release']) + + def setUp(self): + super().setUp() + self.ALL_TASK_NAMES = set([key.replace('_task', '') + for key, _ in self.CIRRUS_YAML.items() + if key.endswith('_task')]) + + def test_dicts(self): + """Expected dictionaries are present and non-empty""" + for name in ('success_task', 'release_task'): + # tests all names then show specific failures + with self.subTest(name=name): + self.assertIn(name, self.CIRRUS_YAML) + self.assertIn(name.replace('_task', ''), self.ALL_TASK_NAMES) + self.assertIn('depends_on', self.CIRRUS_YAML[name]) + self.assertGreater(len(self.CIRRUS_YAML[name]['depends_on']), 0) + + def _check_dep(self, name, task_name, deps): + # name includes '_task' suffix, task_name does not + msg=('Please add "{0}" to the "depends_on" list in "{1}"' + "".format(task_name, name)) + self.assertIn(task_name, deps, msg=msg) + + def test_depends(self): + """Success and Release tasks depend on all other tasks""" + for name in ('success_task', 'release_task'): + deps = set(self.CIRRUS_YAML[name]['depends_on']) + for task_name in self.ALL_TASK_NAMES - self.SUCCESS_RELEASE: + with self.subTest(name=name, task_name=task_name): + self._check_dep(name, task_name, deps) + + def test_release(self): + """Release task must always execute last""" + deps = set(self.CIRRUS_YAML['release_task']['depends_on']) + self._check_dep('release_task', 'success', deps) + + +if __name__ == "__main__": + unittest.main() -- cgit v1.2.3-54-g00ecf