support auto-update flag in pipelines from config repos

[peterrosell]

Issue: #11635

Description: Add support for the auto-update flag to be read and used in materials of pipelines that is configured in config repos.

Question to maintainers: I didn't add this flag as an argument to the constructors of CRPluggableScmMaterial and PluggableSCMMaterialConfig. Instead I used the setters. If you want me to add it to the constructors just let me know and I fix it. It will of course affect more places in the code.

[chadlwilson]

Could you please update/add relevant test assertions as well?

[chadlwilson]

This is actually setting the value underneath on the SCM which is already passed into the constructor via the scmConfig. For clarity, it appears that if anywhere, this should be set earlier directly on the de-duplicated SCM to be less confusing.

But it may however highlight a problem with the scope of auto_update and how it works when multiple pipelines use the same logical material but declare different auto_update values.

We might need to understand what the consequences are for this for determinism of behaviour when creating and merging these - at least to ensure it behaves the same way as the non-pluggable SCMs.

[peterrosell]

Ah, you mean that the code between line 370 and 392 searches for an duplicated scmConfig?
To avoid that we find a scmConfig that we then modify we should set this flag on the scmConfig before doing the search.

So around line 376 add this

            scmConfig.setAutoUpdate(crPluggableScmMaterial.isAutoUpdate());

and revert the change on lines 399-403

[chadlwilson]

Yeah, possibly directly setting the value on the scmConfig prior to constructing is clearer that you might be modifying an existing (shared) config, since the property belongs to the overall config rather than the pluggable config.

But I'll still need to check any implications. In general the way this stuff is modelled in GoCD is a bit problematic due to the de-duplication of SCMs. It's not possible to, say, have one pipeline with a material being manual and another pipeline with the same scm (url, branch) that is auto updating (polling). This causes indeterminism by design, and confusion to users. It's even more confusing for users/admins if there are a mixture of config repo and UI-created pipelines.

The workaround for this nornally is for users to use blanket **/* denylists in individual pipelines' consumption of a material to vary triggering but that won't stop the polling (where people have a preference for webhooks and want to reduce load on their SCM server) - only stop the triggering of certain pipelines.

So I'll want to verify that the way this change behaves is at least similar to git, svn and other SCMs in its determinism/indeterminism.

[peterrosell]

In the method setCommonScmMaterialMembers (line 480) the autoUpdate flag is set for material of UI-created pipelines. That method is called in toScmMaterialConfig for each type of SCM. There is no handling of SCM in that method, maybe it's handled in a different way.

To verify that it doesn't mess up with the other CSMs we could write a test that...

1. have a list of SCMs contains the same material, but with autoUpdate=true
2. call the toPluggableScmMaterialConfig method with autoUpdate=false
3. check that there is a new scmConfig created and not a reused one.

Let me know if there is anything I can give any help or if you want to have a look on your own into this area. It's a lot of details, but I guess you knows the much better than me. :-)

About the workaround. My goal is to reduce the load on the git server as we have a lot of pipelines.

[chadlwilson]

Yup, this fix overall is definitely needed, reasonable and should be able to be made work. I'll try and get some time to play "in anger", and can make the minor tweaks.

Perhaps for the non-plugin ones the de-duplication happens at runtime somehow, or I am misinterpreting the "scope" of the de-duplication that is happening here.

Do you mind sharing which SCM plugin you are using/interested in here? Otherwise I'll probably validate with https://github.com/TWChennai/gocd-git-path-material-plugin since I understand it in depth.

[peterrosell]

I'm using the gocd-yaml-config-plugin and also tested the corresponding json version just to verify that the problem wasn't with the file syntax.

[chadlwilson]

Sorry, I am asking which SCM plugin you are using, i.e which type of custom material you are trying to configure as auto_update: false.

[chadlwilson]

Ive had a play around with this at the weekend, including testing a lot of scenarios as well as comparing to the regular materials.

At the moment I'm not 100% comfortable in the current state because the behaviour is inconsistent with the regular materials. Generally speaking they will warn you if you have inconsistent autoUpdate settings between logically identical materials and force you to make them consistent (big red errors during import).

In this pluggable SCM case it just picks a winner and mutates the underlying shared SCM silently - I believe even before it has been validated to be OK to merge. If you have a config repo that refers to an SCM by its ID, it can also have the effect of changing the setting for an SCM defined via the UI which would be quite unexpected as a side effect of a config repo update.

The challenge here seems to be that the logic to detect for the inconsistent autoUpdate values and force the user to fix them does not run prior to this de-duplication, it happens later when merging the config repo defined pipelines/materials into the main UI/API defined ones. After digging into it, it's unfortunately also pretty clunky and has some weird rules (arguably, bugs) which confused me when testing behaviour of non-plugin SCM materials.

Additionally I am still a bit unsure about the consequences of making a change to the de-duplicated SCM object. This feels like a possible uncontrolled change (mutating shared state as a side effect)

I'll have a think about how we can do this in a way such that config repos can create new SCM objects with this value set, but fail if the autoUpdates are not consistent among all materials (as happens for non-plugin materials) so that no mutation of existing values is possible.

[peterrosell]

Thanks for looking into it this rabbit hole. :-)
It seems a bit odd that material from plugs vs non-plugins are handled differently, but I guess it's due to features have been added a long the road.

Would it be possible to handle the same material that only differs on the autoUpdate as two separate materials? What effects will that have?

[arvindsv]

Would it be possible to handle the same material that only differs on the autoUpdate as two separate materials? What effects will that have?

I think the challenge will be something called the "fingerprint" of the material. The de-duplication that Chad mentioned is done by generating a fingerprint (a hash, essentially) for every material and then de-duplicating them. Doing what you asked about (handling them as two separate materials) will be a very fundamental change that will have massive consequences. To add: every unique material has a single "flyweight" directory, in which a git bare clone happens. That will be one area that will immediately be affected by having two materials essentially having the same fingerprint.

Now, back to this change: I share Chad's concerns, which he has articulated well. Chad is right that your change is the right thing to do, but the consequences for existing setups is something we need to think about. Even if the current implementation is buggy.

The challenge here seems to be that the logic to detect for the inconsistent autoUpdate values and force the user to fix them does not run prior to this de-duplication, it happens later when merging the config repo defined pipelines/materials into the main UI/API defined ones.

I haven't looked at the code, but I see what you mean. I hesitate to point to #1133, but I will just for fun. :) One of the principles when we were discussing this feature was to make sure that a syntactically valid config XML will not make the server startup fail due to missing config-repo pipelines. I think there were some checks that were deferred due to that, which could be causing the issue you mention.

I'll have a think about how we can do this in a way such that config repos can create new SCM objects with this value set, but fail if the autoUpdates are not consistent among all materials (as happens for non-plugin materials) so that no mutation of existing values is possible.

If we can reproduce the case of the mutation already happening, then we can make this change and see if it makes anything worse / different, and see if that's a potential temporary "fix" that's not worse than the bug that exists. However, I share your caution / trepedation in this area. It can go very wrong if we make a mistake.

If not, one approach would be to put in a check early on somewhere for this specific condition, and then deal with the problem of existing configs which have this case / mismatch failing after an upgrade.

[chadlwilson]

Would it be possible to handle the same material that only differs on the autoUpdate as two separate materials? What effects will that have?

I think the challenge will be something called the "fingerprint" of the material. The de-duplication that Chad mentioned is done by generating a fingerprint (a hash, essentially) for every material and then de-duplicating them. Doing what you asked about (handling them as two separate materials) will be a very fundamental change that will have massive consequences. To add: every unique material has a single "flyweight" directory, in which a git bare clone happens. That will be one area that will immediately be affected by having two materials essentially having the same fingerprint.

Right now, there is already something weird that is possible here. You can add a pluggable SCM material via the UI, and then another with the same fingerprint via a config repo which ends up duplicating. So one "middle ground" given it seems already possible might be to allow one from each rather than aiming for global de-duplication which is where the complexity comes from (it means you need to merge all materials - those from UI and those implied by N config repos - before you can determine if there are conflicts).

Pluggable SCMs have a bit more flexibility than regular materials due to the way they were modelled (which itself creates a lot of complexity in the UI - probably why people generally avoid pluggable SCM materials - they are a pain to click ops, and MUCH easier via config repos) As long as GoCD itself seems them as separate pluggable SCMs, we are "probably" OK, but testing all the peripheral cases like webhooks is a bit troublesome.

But yeah, the whole area is a bit complex, honestly speaking. Since materials are so important as an independent concept in GoCD, which gives it its VSM magic, I guess that's "par for the course".

The challenge here seems to be that the logic to detect for the inconsistent autoUpdate values and force the user to fix them does not run prior to this de-duplication, it happens later when merging the config repo defined pipelines/materials into the main UI/API defined ones.

I haven't looked at the code, but I see what you mean. I hesitate to point to #1133, but I will just for fun. :) One of the principles when we were discussing this feature was to make sure that a syntactically valid config XML will not make the server startup fail due to missing config-repo pipelines. I think there were some checks that were deferred due to that, which could be causing the issue you mention.

Thanks for this, yeah I have seen #1133 and dug into it, as well as the subsequent reworks over the years. There are some existing cases where the config can become invalid due to external changes (#11947) which somewhat violate this (server does start, but now config is invalid and won't save).

I'll have a think about how we can do this in a way such that config repos can create new SCM objects with this value set, but fail if the autoUpdates are not consistent among all materials (as happens for non-plugin materials) so that no mutation of existing values is possible.

If we can reproduce the case of the mutation already happening, then we can make this change and see if it makes anything worse / different, and see if that's a potential temporary "fix" that's not worse than the bug that exists. However, I share your caution / trepidation in this area. It can go very wrong if we make a mistake.

If not, one approach would be to put in a check early on somewhere for this specific condition, and then deal with the problem of existing configs which have this case / mismatch failing after an upgrade.

I can definitely reproduce this case with the PR code as it currently is. It's quite clear that due to the lack of immutability on the partial configs generated from all the config-repos pre-merge, one has to be very careful when "touching" them.

Nevertheless, full disclosure - while I am still interested in finding a way to fix/support this (especially with my earlier hat on as main developer of the modern https://github.com/TWChennai/gocd-git-path-material-plugin which could benefit a lot from this when used with config repos), I might struggle to get this done for 23.2.0 which I have been procrastinating on releasing for a while. So don't be surprised if I have to bump it for later 😢

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 14 days. It will be closed in 7 days if no further activity occurs.
Thank you for your contributions!