Skip to content

Conversation

@camrynl
Copy link
Contributor

@camrynl camrynl commented Oct 18, 2022

Reason for Change:

ACN PR pipeline is complex and contains multiple stages where the repo is checked out. A common issue we've been seeing in the pipeline is failures at the publish manifest/testing stages that occur when a pr is merged to master while another pr is validating the pipeline checks. Pushes to master cause a change in the commit tag that is used to label images, which leads to conflict because the new tag images are not found. The commit tag must change because we must always be testing against the latest version of master in order for the checks to be true.

PRs are required to be up to date to master, want to cancel runs when branch is found to be out of date

Current state of PR involves checking the tags between build/publish/test stages of the pipeline. Need to validate this condition against multiple pipeline runs to see how it behaves.

Issue Fixed:

Requirements:

Notes:

@camrynl camrynl added work-in-progress ci Infra or tooling. labels Oct 18, 2022
@camrynl
Copy link
Contributor Author

camrynl commented Oct 21, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@camrynl
Copy link
Contributor Author

camrynl commented Oct 21, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@camrynl
Copy link
Contributor Author

camrynl commented Oct 21, 2022

/azp run

@camrynl
Copy link
Contributor Author

camrynl commented Oct 27, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@camrynl
Copy link
Contributor Author

camrynl commented Oct 27, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

aegal
aegal previously approved these changes Oct 27, 2022
Copy link
Contributor

@aegal aegal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this a smart way to workaround this issue. It is unfortunate that ADO does not offer this natively.

@camrynl
Copy link
Contributor Author

camrynl commented Oct 27, 2022

I think this a smart way to workaround this issue. It is unfortunate that ADO does not offer this natively.

it would be so much simpler if ADO did offer cancellation. I haven't been able to validate these changes because even as PRs are merged, the build is not canceled. It is also difficult to see what's going on with the tags because it requires a PR to be merged during the pipeline run in order for me to see what happens.

@aegal
Copy link
Contributor

aegal commented Oct 27, 2022

I think this a smart way to workaround this issue. It is unfortunate that ADO does not offer this natively.

it would be so much simpler if ADO did offer cancellation. I haven't been able to validate these changes because even as PRs are merged, the build is not canceled. It is also difficult to see what's going on with the tags because it requires a PR to be merged during the pipeline run in order for me to see what happens.

@aegal aegal closed this Oct 27, 2022
@aegal
Copy link
Contributor

aegal commented Oct 27, 2022

@aegal aegal reopened this Oct 27, 2022
@aegal
Copy link
Contributor

aegal commented Oct 27, 2022

I think this a smart way to workaround this issue. It is unfortunate that ADO does not offer this natively.

it would be so much simpler if ADO did offer cancellation. I haven't been able to validate these changes because even as PRs are merged, the build is not canceled. It is also difficult to see what's going on with the tags because it requires a PR to be merged during the pipeline run in order for me to see what happens.

One thing you can do is remove the sleep 2m for now that eventually leads to the build failing, check it in and see if any of the other p.r's lead to the timeout_cancel step being issued, that should give you confidence in it and not require you to run /azp run over and over to catch it, then open another P.R that just adds back the sleep when you see it triggered and happy with it

@camrynl
Copy link
Contributor Author

camrynl commented Oct 27, 2022

I think this a smart way to workaround this issue. It is unfortunate that ADO does not offer this natively.

it would be so much simpler if ADO did offer cancellation. I haven't been able to validate these changes because even as PRs are merged, the build is not canceled. It is also difficult to see what's going on with the tags because it requires a PR to be merged during the pipeline run in order for me to see what happens.

One thing you can do is remove the sleep 2m for now that eventually leads to the build failing, check it in and see if any of the other p.r's lead to the timeout_cancel step being issued, that should give you confidence in it and not require you to run /azp run over and over to catch it, then open another P.R that just adds back the sleep when you see it triggered and happy with it

Right now I suspect that the tag variables are holding the same values even though a new PR is merged. With the most recent merge to master, the validation step was skipped but the merge had occurred while my pipeline was in the build stage. I want to verify that the tags are different, because this is what we need to check for the validation step. Maybe I can check in this PR after removing sleep 2m and the condition that tags are NE in order to read the tags between stages on every run to see if it is actually changed.

@camrynl camrynl requested a review from aegal November 1, 2022 23:26
@estebancams
Copy link
Contributor

estebancams commented Nov 4, 2022

If we move forward with this approach, should we also add the same checks between test and buildbinaries/buildimages stages too? My concern here is to be building something that was not tested

@camrynl
Copy link
Contributor Author

camrynl commented Nov 7, 2022

If we move forward with this approach, should we also add the same checks between test and buildbinaries/buildimages stages too? My concern here is to be building something that was not tested

The check is happening in two places-- after build images and after publish manifests. Build binaries/Build images happen concurrently so I don't think we need the check there as well. Let's discuss this PR in sync today

@camrynl camrynl enabled auto-merge (squash) November 14, 2022 16:44
@camrynl camrynl merged commit b0efb9c into Azure:master Nov 14, 2022
rjdenney pushed a commit to rjdenney/azure-container-networking that referenced this pull request Jan 19, 2023
* test repo trigger

* add resources label

* add repositories under resources

* test with endpoint

* testing with tag check

* typo in condition line

* evaulate version in condition

* compare tag vars

* test cancellation stage

* add job to cancel stage

* change name of cancel stage

* add pool and msg to cancel job

* change cancellation dependency

* add second tag validation stage

* spcaing ofvalidation1

* test ne and cancellation script

* check tag versions

* set currentTagx variable

* test new tag var

* fix value mapping

* check_tag job

* update current tag in manifest stage

* reading tags

* adjust variable assignment

* uncomment sleep

* read out different tags

* test validation dependency

* use succeeded/failed in condition check

* fix condition

* fix call to succeeded()

* omit condition on validation stages to read tags

* remove sleep, just validate condition

* fix error with submod swift naming
@camrynl camrynl deleted the pipelineTriggers branch September 28, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Infra or tooling. work-in-progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants