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

extraConfigs #77

Merged

Conversation

itamar-marom
Copy link
Collaborator

@itamar-marom itamar-marom commented Jun 12, 2023

Fixes #76
Fixes #75

Description

New field in the API:

// References to ConfigMaps holding more files to mount to the CommonConfigMountPath.
// +optional
ExtraCommonConfig []*v1.ObjectReference `json:"extraCommonConfig"`

Each ConfigMap can hold as many extra files as the user wants.
The operator will go over the ConfigMap object, then will fetch all file names (key) and their content (value) from the ConfigMaps. It will add them to the common ConfigMap that is being mounted into the _common directory.

Had to refactor e2e to work smoothly without unnecessary long sleep commands + Added an E2E test to check the new feature.

Option 1: Mounting files as subPath - not good enough:
Kubernetes documentation states, A container using a ConfigMap as a [subPath](https://kubernetes.io/docs/concepts/storage/volumes/#using-subpath) volume will not receive ConfigMap updates.

Options 2: the above solution

  • Added a new field: ExtraCommonConfig
  • Moved all configuration-related functions to configuration.go
  • Created a new function called addExtraCommonConfig that is being called in makeCommonConfigMap

This PR has:

  • been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
  • been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Key changed/added files in this PR
  • configuration.go
  • Druid API
  • e2e

@itamar-marom itamar-marom marked this pull request as draft June 12, 2023 13:09
}, nil
}

func (r *DruidReconciler) makeCommonConfigMap(ctx context.Context, m *v1alpha1.Druid, ls map[string]string) (*v1.ConfigMap, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets not add methods now. This will need a big change on all the code structure. Not just configmaps.
Ideally we would want another interface as druidBuilder to expose this methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it from the Reconciler object. Will fix the e2e and ping you

@itamar-marom itamar-marom marked this pull request as ready for review June 13, 2023 11:49
@itamar-marom
Copy link
Collaborator Author

@AdheipSingh ready with no new methods for the reconcile object.
I do wish to discuss about refactoring as the controller becomes more and more complicated.

@itamar-marom
Copy link
Collaborator Author

Generally, with this feature, we can then eliminate all other "common" fields and rename this to commonConfig which will be built from other ConfigMaps.

@itamar-marom
Copy link
Collaborator Author

Just updated the new chart version since it includes updated crd

@AdheipSingh
Copy link
Contributor

Thanks @itamar-marom . Merging it .

@AdheipSingh AdheipSingh merged commit 69a7094 into datainfrahq:master Jul 4, 2023
1 check passed
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.

Ability to add custom files in _common directory Support Hadoop Indexing
2 participants