Conversation
|
Images are ready for the commit at 30c8e64. To use with deploy scripts, first |
There was a problem hiding this comment.
Seems reasonable approach.
I want to point out that we are releasing 3.73.1 patch which include fixes for other issues as well.
What does it mean?
Some customers (with the small environment) will most likely have Central up and running. However, we are recommending patch for other issues as well. Therefore, after applying 3.73.1, the behaviour introduced in 3.73 will change.
What can we do?
I think we should document the change in changelog. That is, basically capturing the rationale in ROX-11657, something like:
## [3.73.1]
### Added Features
### Removed Features
### Deprecated Features
### Technical Changes
3.73 introduced a change to ACS autogenerated image integration workflows.
However, this change in workflow caused issue...(details [here](https://access.redhat.com/node/6990153)).
To fix the issue introduced in 3.73, 3.73.1 will reinstate the old workflow....
Therefore, autogenerated integrations may not work successfully in environments where many different credentials
are used for multiple repos within the global repository.
janisz
left a comment
There was a problem hiding this comment.
Do you plan to add regression test for this?
| // Only upsert autogenerated integrations with a source if the feature is enabled. | ||
| if !features.SourcedAutogeneratedIntegrations.Enabled() && ii.GetAutogenerated() && ii.GetSource() != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
My mental boolean compiler claims the code and the comment do not quite match. Applying De Morgan's rule:
if features.SourcedAutogeneratedIntegrations.Enabled() || !ii.GetAutogenerated() || ii.GetSource() == nil {
//... upsert
}
This reads to me as "Upsert all if feature is enabled, otherwise skip autogenerated and those with non-nil source". The autogenerated with non-nil source indeed will be inserted only when feature is enabled but what about non-autogenerated with non-nil source?
There was a problem hiding this comment.
The Source field was added into 73 and its addition has caused some issues. The goal is to ensure when the feature flag is turned off, we revert back to the old implementation, which inserted all autogenerated registries. The Source field should not be populated when the flag is off, so this check is meant to ensure that, if it did somehow pass through, only registries without Source are upserted
| } | ||
| source := imageIntegration.GetSource() | ||
| if source == nil { | ||
| if !features.SourcedAutogeneratedIntegrations.Enabled() || source == nil { |
There was a problem hiding this comment.
I thought we don't speak about autogenerated when the flag is disabled?
There was a problem hiding this comment.
This flag is specifically for the new version of autogenerated registries introduced in 3.73.0: ones with this Source field. We have a separate environment setting (not feature flag) to ignore all autogenerated registry integrations: https://github.com/stackrox/stackrox/blob/master/pkg/env/autogenerated_registries.go
dhaus67
left a comment
There was a problem hiding this comment.
One clarifying comment on how autogenerated registries are re-synced after deleting them on startup, otherwise LGTM.
|
|
||
| var attempted, deleted int | ||
| for _, ii := range iis { | ||
| if ii.GetAutogenerated() && ii.GetSource() != nil { |
There was a problem hiding this comment.
When there's customer who migrate from 3.72.0 -> 3.73.0, they will have the mix of autogenerated integrations with Source != nil and Source == nil.
In case of a "clean" installation of 3.73.0, all autogenerated image integrations will have Source != nil set.
So, we will delete all autogenerated image integrations then.
In that case, how will the recovery look like for this? Will it be eventual consistent such that the autogenerated registries we expect to have (i.e. for internal OpenShift registry) will show up after some time, or does that require bumps of sensors to send the autogenerated registries once more?
(I honestly don't think that the OpenShift internal ones are that important from an autogenerated image integration perspective, unless central can reach them, which won't be the case for most of them)
There was a problem hiding this comment.
In the case of a clean install, I'd expect us to eventually find all the autogenerated registries. When it comes to OpenShift internal registry, we do not use the stuff sent to Central for that
Co-authored-by: Alex Rukletsov <alexr@stackrox.com>
|
@dhaus67 @RTann, help me understand this. The behavior change for auto-generated image integrations was released in 3.73.0. Therefore, until Central gRPC listeners are ready and start receiving messages from Sensor, Central DB should not have had many image integrations because Central is beginning to receive messages from Sensor past the readiness probe. Is the above understanding correct?
|
I agree with your assessment. I am not sure if the cases we found were created after Central had already restarted due to some reason or if this was the first 3.73.0 upgrade attempt. |
Okay, let's run by that theory. |
|
Please merge the changes to branch |
…eature flag (#4160) minus the changes to central/image/service/service_impl.go
Description
This change adds a feature flag around the changes introduced in #2572.
Checklist
[ ] Unit test and regression tests added[ ] Determined and documented upgrade steps[ ] Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)Testing Performed
Given:
config.json:create-kubernetes-secrets.sh:dockerconfigjsonsecrets viacreate-kubernetes-secrets.shpkg/registries/docker