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

Run single instance of a pipeline without locking at end (unlock when done) (#106) #3943

Merged
merged 14 commits into from Oct 30, 2017

Conversation

Projects
None yet
3 participants
@arvindsv
Copy link
Member

commented Oct 17, 2017

The ability to unlock a pipeline automatically, upon failure. So, run a single instance of a pipeline run but don't lock if it fails. See #106.

Todos:

This image is to help @jyotisingh decipher this test:
image

This is how it looks in the old config edit (rails):
2017_10_17_21-26-21

This is how it looks in the new config edit (spa):
2017_10_17_21-26-42

With tooltip:
2017_10_17_21-30-23

Changes in API:

Pipeline config API v5 has been created. Instead of: "enable_pipeline_locking": true or "enable_pipeline_locking": false, you need to use: lockBehavior with one of these three values:

  • lockOnFailure (same as the old true)
  • unlockWhenFinished (this is new) or
  • none (same as the old false).

Changes in config XML:

This:

<pipeline name="pipeline1" isLocked="true">
  ...
</pipeline>

is automatically migrated to:

<pipeline name="pipeline1" lockBehavior="lockOnFailure">
  ...
</pipeline>

Also, this:

<pipeline name="pipeline1" isLocked="false">
  ...
</pipeline>

is automatically migrated to:

<pipeline name="pipeline1" lockBehavior="none">
  ...
</pipeline>

The other new option is: lockBehavior="unlockWhenFinished".

@arvindsv arvindsv added this to the Release 17.12 milestone Oct 17, 2017

@arvindsv arvindsv requested a review from jyotisingh Oct 17, 2017

@arvindsv arvindsv force-pushed the arvindsv:unlock-upon-failure branch from 4120533 to 706997a Oct 18, 2017

@arvindsv arvindsv changed the title WIP: Unlock upon failure (#106) WIP: Unlock pipeline when done (#106) Oct 18, 2017

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2017

Checked with this config:

<pipeline name="test">
  <materials>
    <git url="/tmp/test.git" />
  </materials>
  <stage name="stage1">
    <jobs>
      <job name="stage1_job1">
        <tasks>
          <exec command="bash">
            <arg>-c</arg>
            <arg>while [ ! -f "/tmp/p/${GO_PIPELINE_LABEL}/1" ]; do sleep 1; date; done; exit $(cat "/tmp/p/${GO_PIPELINE_LABEL}/1")</arg>
            <runif status="passed" />
          </exec>
        </tasks>
      </job>
    </jobs>
  </stage>
  <stage name="stage2">
    <jobs>
      <job name="stage2_job1">
        <tasks>
          <exec command="bash">
            <arg>-c</arg>
            <arg>while [ ! -f "/tmp/p/${GO_PIPELINE_LABEL}/2" ]; do sleep 1; date; done; exit $(cat "/tmp/p/${GO_PIPELINE_LABEL}/2")</arg>
            <runif status="passed" />
          </exec>
        </tasks>
      </job>
    </jobs>
  </stage>
  <stage name="stage3">
    <approval type="manual" />
    <jobs>
      <job name="stage3_job1">
        <tasks>
          <exec command="bash">
            <arg>-c</arg>
            <arg>while [ ! -f "/tmp/p/${GO_PIPELINE_LABEL}/3" ]; do sleep 1; date; done; exit $(cat "/tmp/p/${GO_PIPELINE_LABEL}/3")</arg>
            <runif status="passed" />
          </exec>
        </tasks>
      </job>
    </jobs>
  </stage>
</pipeline>

and controlled it with:

LABEL=2; STAGE=1; STATUS=0; mkdir -p "/tmp/p/${LABEL}"; echo "${STATUS}" > "/tmp/p/${LABEL}"/"${STAGE}"

@arvindsv arvindsv force-pushed the arvindsv:unlock-upon-failure branch from 00b5f47 to b051164 Oct 20, 2017

@arvindsv arvindsv changed the title WIP: Unlock pipeline when done (#106) WIP: Unlock pipeline when done (run single instance of a pipeline without locking at end) (#106) Oct 20, 2017

@arvindsv arvindsv changed the title WIP: Unlock pipeline when done (run single instance of a pipeline without locking at end) (#106) WIP: Run single instance of a pipeline without locking at end (unlock when done) (#106) Oct 20, 2017

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2017

@rajiesh The only part that is left in this is writing a new functional test for this behavior. Should I try and write a new one, or do you think we can add this behavior to an existing test?

@jyotisingh
Copy link
Contributor

left a comment

There is one behavior that we need to decide/document.

  • So, with this PR v4 continues to work as is ie. return booleans for lockBehavior and v5 starts allowing one of the three values to be specified for the lockBehavior which is cool. But what if someone updated the pipeline config from quick-edit/old-UI/apiv5 to set it to unlockWhenFinished, and another person queried for the config using apiv4 (returns the boolean value, false in this case), edited a totally unrelated field say material and updates the config. This would unintentionally revert the changes made by person 1.

Spun up a local server with this PR, things look fine to me.
The UI option states Run single instance of pipeline at a time and Run single instance of pipeline and lock on failure which I think are very clear, but I am not sure I can say the same for the strings unlockWhenFinished and lockOnFailure which are exposed in the config xml and API.


public ConfigRepoMigrator() {
targetVersionToJoltTransformFile = new HashMap<>();
targetVersionToJoltTransformFile.put(2, "/v1_to_v2_enable_pipeline_locking_to_lock_behavior.json");

This comment has been minimized.

Copy link
@jyotisingh

jyotisingh Oct 26, 2017

Contributor

Would be nicer if we could move that json migration file from being just another resource to a appropriately named folder within the resources folder with files such as 2.json, 3.json and so on, so we do not have keep updating this map when newer migrations are added. This would also help with all migrations to be consolidated at one single place instead of getting mixed up with other resources (I know this is the only resource available at this point).
What I mean is targetVersionToJoltTransformFile would go away and this line could effectively become like this:

String targetVersionFile = String.format("/config-repo/migrations/%s.json", targetVersion); // /config-repo/migrations/2.json
String transformJSON = IOUtils.toString(this.getClass().getResourceAsStream(targetVersionFile), "UTF-8");

instead of :

String targetVersionFile = targetVersionToJoltTransformFile.get(targetVersion); // /v1_to_v2_enable_pipeline_locking_to_lock_behavior.json
String transformJSON = IOUtils.toString(this.getClass().getResourceAsStream(targetVersionFile), "UTF-8");

PS: I couldn't find any issues with your PR so nitpicks are all I have.

This comment has been minimized.

Copy link
@arvindsv

arvindsv Oct 26, 2017

Author Member

Done.

@@ -73,7 +75,7 @@ public CRParseResult responseMessageForParseDirectory(String responseBody) {
int version = responseMap.target_version;

while (version < CURRENT_CONTRACT_VERSION) {
migrate(responseBody, version);
responseBody = migrate(responseBody, version + 1);

This comment has been minimized.

Copy link
@jyotisingh

jyotisingh Oct 26, 2017

Contributor

That works or just move version++; before this one.

This comment has been minimized.

Copy link
@arvindsv

arvindsv Oct 26, 2017

Author Member

Done.

@@ -231,6 +235,13 @@ private boolean validateLabelTemplateTruncation(String labelTemplate) {
return LABEL_TEMPATE_ZERO_TRUNC_BLOCK_PATTERN.matcher(labelTemplate).find();
}

private void validateLockBehaviorValues() {

This comment has been minimized.

Copy link
@jyotisingh

jyotisingh Oct 26, 2017

Contributor

While it makes sense to add this validation in the domain class, may be we could preempt it in CRPipeline#getErrors for the pipelines defined in external config-repo.

This comment has been minimized.

Copy link
@arvindsv

arvindsv Oct 26, 2017

Author Member

Done.

This comment has been minimized.

Copy link
@arvindsv

arvindsv Oct 26, 2017

Author Member

I've left it in the PipelineConfig class as well. Feels more correct.

This comment has been minimized.

Copy link
@arvindsv

arvindsv Oct 26, 2017

Author Member

I mean, it's now in both places, CRPipeline and PipelineConfig.

This comment has been minimized.

Copy link
@jyotisingh

jyotisingh Oct 26, 2017

Contributor

Yes, that's exactly what I was asking for.

@rajiesh

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

@arvindsv We can write new specs for this. I Can work on it on Monday

arvindsv added some commits Oct 2, 2017

#106 - Migrate "isLocked" to "lockBehavior" for pipeline
* Convert:
  - <pipeline name="abc" isLocked="true"/>  => <pipeline name="abc" lockBehavior="lockOnFailure"/>
  - <pipeline name="abc" isLocked="false"/> => <pipeline name="abc" lockBehavior="none"/>
  - <pipeline name="abc"/>                  => <pipeline name="abc"/>

* Values of lockBehavior are not yet constrained through XSD.
#106 - Restrict lockBehavior tag values
* Valid values are: lockOnFailure, unlockWhenFinished, none
#106 - Refactor
* Rename isLock => isLockable
* Rename isPipelineLocked => isPipelineLockable
* Remove unused method: removeExplicitLocks
#106 - Add new unlock behavior
* When the new "unlockWhenFinished" flag is set, unlock the pipeline when the pipeline is
  finished (either success or failed) or when the first manual stage is reached. This
  should simulate a single instance run, without locking at the end.
#106 - Introduce API v5 - Part 1
Just files copied over from v4. No changes
#106 - Introduce API v5 - Part 2
Change all v4 to v5
#106 - Change pipeline config API v5
Now accepts "lock_behavior" instead of "enable_pipeline_locking".
#106 - Make pipeline config SPA work (quick edit)
* Use pipeline config API v5
* Change all "enable_pipeline_locking" to "lock_behavior"

@arvindsv arvindsv force-pushed the arvindsv:unlock-upon-failure branch from a95f40f to 441b34e Oct 26, 2017

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2017

@jyotisingh If you have suggestions for unlockWhenFinished and lockOnFailure, I can change them. Will need to change in the plugins, functional tests and docs PRs as well, but I can. Let me know what sounds better.

lockBehavior=...

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2017

About that case (v4 and v5), it's expected that you will overwrite it. That's why you shouldn't use older versions. :) There's no way (without changing v4's code) that this can be handled, right? I don't think putting in code to detect this in v5 sounds right, to me.

@jyotisingh

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

Regarding names - No, I don't have a better suggestion in mind. Thought of making you come up with one, but if you can't think of anything else, then let's stick to it.

Regarding v4 &v5 - I can't think of a way to get around the problem so thought may be we should call it out. Not sure if release notes is the right place or GitHub issue - of course it would be a "won't fix" with reasons. But we wouldn't be able to find this discussion between us when someone reports it at a later point.

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2017

Ok, I opened and closed #3958 for this.

@jyotisingh

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

Cool, PR looks good to me. Run the PR pipeline. Will merge it when the build passes.

arvindsv added some commits Oct 17, 2017

#106 - Make config repo (pipelines as code) work
* Use a JSON/jolt migration to migrate from v1 (enable_pipeline_locking) to v2 (lock_behavior).
#106 - Move file atomically in test
Attempt to reduce flakiness of plugin monitor test. The hypothesis is that the file copy is showing up as two events, one
an add and another an update, because it is not atomic.
#106 - Validate lockBehavior values before pipeline save
Without this, config repo pipelines are allowed to have invalid lockBehavior values.

@arvindsv arvindsv force-pushed the arvindsv:unlock-upon-failure branch from 441b34e to f6d2562 Oct 26, 2017

@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2017

The pipeline seems ok. The functional tests need my PR to work properly.

@jyotisingh jyotisingh changed the title WIP: Run single instance of a pipeline without locking at end (unlock when done) (#106) Run single instance of a pipeline without locking at end (unlock when done) (#106) Oct 27, 2017

@jyotisingh jyotisingh merged commit ce15cda into gocd:master Oct 30, 2017

8 checks passed

build-linux-PR/build-non-server
Details
build-linux-PR/build-server
Details
build-windows-PR/build-non-server
Details
build-windows-PR/build-server
Details
installers-PR/dist
Details
license/cla Contributor License Agreement is signed.
Details
plugins-PR/build
Details
trigger/do-nothing
Details
@arvindsv

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2017

@jyotisingh - At some point, do you mind merging these three PRs?

gocd/api.go.cd#213
gocd/docs.go.cd#122
gocd/plugin-api.go.cd#44

Not very urgent.

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.