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

1496 asset sync namespaces #1519

Merged
merged 4 commits into from
Feb 17, 2020
Merged

1496 asset sync namespaces #1519

merged 4 commits into from
Feb 17, 2020

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Feb 14, 2020

Ref : #1496

With this change, the controller spawns cronjobs/jobs in the kubeapps namespace and successfully sync's public app repositories across all namespaces.

I also noticed that I was initially unable to delete app repositories in the kubeapps namespace, they'd stay in a deleting state with a finalizer. Turned out to be because the app repo controller was recreating cronjobs as soon as k8s was deleting them (as they were owned by the app repo).

The main changes are:

  • extra cli arg when the feature flag is set, which updates the informer to receive events on app repositories cluster wide,
  • Updating the controller struct to include a field for the kubeapps namespace so we can differentiate,
  • Updating so that OwnerReferences are only added for resources created in the same namespace as the app repository (ie. only for repos in the kubeapps namespace), as owner references are only allowed for resources in the same namespace
  • Allow controller to recognise a parent app repo even if it's not an owner, to avoid creating events for an app repo in another namespace (for now).

TODO:

  • I QA'd with the switch on to verify it works, but haven't gone back and QA'd to ensure things work fine without the flag (ran out of time)
  • Ensure for app repos from other namespaces with repo creds that the secret is included with the job (from the kubeapps copy).

@absoludity absoludity self-assigned this Feb 14, 2020
@@ -367,26 +385,40 @@ func (c *Controller) handleObject(obj interface{}) {
return
}

if apprepo.ObjectMeta.DeletionTimestamp != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required so that we don't re-process (ie. re-create cronjobs etc.) for an app repo that is being deleted.

// cronjob for an app repo in another namespace, then we should
// log a warning to the event recorder and return it.
if !metav1.IsControlledBy(cronjob, apprepo) && !objectBelongsTo(cronjob, apprepo) {
log.Errorf("Cronjob: %+v\nAppRepo: %+v", cronjob.ObjectMeta, apprepo.ObjectMeta)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this for debugging purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - thanks :)

@@ -17,6 +17,7 @@ func Test_newCronJob(t *testing.T) {
dbName = "assets"
dbUser = "admin"
dbSecretName = "mongodb"
const kubeappsNamespace = "kubeapps"
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes? Not sure what you mean here. I could either create a var with:

kubeappsNamespace := "kubeapps"

or set a constant string, as I have, which given I'm not modifying it... Let me know if I missed your meaning.

return &batchv1.Job{
ObjectMeta: metav1.ObjectMeta{
GenerateName: cronJobName(apprepo) + "-",
Namespace: apprepo.Namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove the namespace? (to avoid duplication?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because we choose which namespace to write these resources in the actual client call, so if we also write it here, we have to duplicate the logic/args to ensure it is the same (otherwise, the k8s api server rejects it as it should).

@absoludity
Copy link
Contributor Author

* I QA'd with the switch on to verify it works, but haven't gone back and QA'd to ensure things work fine without the flag (ran out of time)

I QA'd this and had to do a small change in 8181081 so that when the feature flag is not set, we request apprepos for the kubeapps namespace after deleting one.

@absoludity absoludity merged commit e8ad3e4 into master Feb 17, 2020
@absoludity absoludity deleted the 1496-asset-sync-namespaces branch February 17, 2020 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants