-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add ability to install porter plugins #108
Conversation
9f74f91
to
c9459f5
Compare
28795f0
to
2a6432f
Compare
Codecov Report
@@ Coverage Diff @@
## main #108 +/- ##
==========================================
- Coverage 78.30% 75.58% -2.72%
==========================================
Files 12 13 +1
Lines 1014 1536 +522
==========================================
+ Hits 794 1161 +367
- Misses 138 242 +104
- Partials 82 133 +51
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
99c682c
to
0b6a224
Compare
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
493c18f
to
e2c7bab
Compare
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
e2c7bab
to
db360f2
Compare
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
// TODO: once porter has ability to install multiple plugins with one command, we will allow users | ||
// to install multiple plugins. Currently, only the first item defined in the plugin list will be | ||
// installed. | ||
updatedCfg := setDefaultPlugins(agentCfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-adding comment from last review since I think it was missed:
I recommend moving most of the detailed logic from Reconcile into multiple functions. If you look over the other reconcile implementations in the other controller, I've tried real hard to keep them short/readable, sticking to the high level status transitions, because otherwise this function is overwhelming and difficult to test/maintain.
…gent config and other porter CRDs Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
return porterv1.AgentConfigSpecAdapter{}, errors.Wrapf(err, "cannot retrieve agent configuration %s specified by the agent action", action.Spec.AgentConfig.Name) | ||
|
||
} | ||
isCfgReady = err == nil && instCfg.Status.Ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this is a giant feature. I had no idea when we first started that it would require so much careful logic and design to work properly.
Thank you for taking the time to think through all the workflow and edge cases. This is amazing and I'm really excited to get it merged so everyone can use it! 💖
api/v1/agentconfig_types.go
Outdated
@@ -175,6 +102,9 @@ func (c AgentConfigSpec) MergeConfig(overrides ...AgentConfigSpec) (AgentConfigS | |||
// AgentConfigStatus defines the observed state of AgentConfig | |||
type AgentConfigStatus struct { | |||
PorterResourceStatus `json:",inline"` | |||
// The current status of the . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this comment is incomplete
api/v1/agentconfig_types.go
Outdated
@@ -175,6 +102,9 @@ func (c AgentConfigSpec) MergeConfig(overrides ...AgentConfigSpec) (AgentConfigS | |||
// AgentConfigStatus defines the observed state of AgentConfig | |||
type AgentConfigStatus struct { | |||
PorterResourceStatus `json:",inline"` | |||
// The current status of the . | |||
// +kubebuilder:validation:Type=boolean | |||
Ready bool `json:"ready,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I recommend removing omitempty, so that when people look at the status they see all the fields and their current values. As it is, this (hiding ready when false) requires the user to know the schema and that ready defaults to false.
Version string `json:"version,omitempty" mapstructure:"version,omitempty"` | ||
} | ||
|
||
// AgentConfigSpecAdapter is a wrapper of AgentConfigSpec with a list representation of plugins configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring seems a bit misleading since it says that the adapter exists to just provide Plugins as a list instead of a map. But based on our previous discussions around the difficulties in defaulting the plugins, and how you are using the original
field to protect people from accidentally directly accessing the fields instead of using the getter functions, makes me think that there's a lot more behind why you split this into it's own data structure.
Can you add a bit of that to the doc so that people can follow why this struct exists and how they should use it?
if p.FeedURL != "" { | ||
plugins = append(plugins, fmt.Sprintf("_%s", cleanURL(p.FeedURL))) | ||
} | ||
if p.URL != "" { | ||
plugins = append(plugins, fmt.Sprintf("_%s", cleanURL(p.URL))) | ||
} | ||
if p.Mirror != "" { | ||
plugins = append(plugins, fmt.Sprintf("_%s", cleanURL(p.Mirror))) | ||
} | ||
if p.Version != "" { | ||
plugins = append(plugins, fmt.Sprintf("_%s", p.Version)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion, but it may be worthwhile for maintenance of this code and the function above to split these lines into a separate function and then use the value for both the label and the hash.
api/v1/agentconfig_types.go
Outdated
// GetLabels returns a value that is safe to use | ||
// as a label value and represents the plugin configuration used | ||
// to trigger reconciliation. | ||
// labels are restricted to alphanumeric and .-_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one more restriction on label values, they must be 63 characters or less
require.True(t, apierrors.IsNotFound(err), "expected the agent config was deleted") | ||
|
||
// Verify that reconcile doesn't error out after it's deleted | ||
triggerReconcile() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phew! This is a mega test 💯, looks great and just shows how much logic is tied up in the AgentConfigController!
docs/content/quickstart/_index.md
Outdated
porterRepository: ghcr.io/getporter/porter-agent | ||
porterVersion: v1.0.2 | ||
serviceAccount: porter-agent | ||
volumeSize: 64Mi | ||
pullPolicy: Always | ||
installationServiceAccount: installation-agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we omit all of these and just specify a plugin?
var porterAgentImgRepository = "ghcr.io/getporter/dev/porter-agent-kubernetes" | ||
var porterAgentImgVersion = porterVersion | ||
var ( | ||
porterAgentImgRepository = "ghcr.io/getporter/porter-agent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WOOOOOO! 🎉 I love seeing this.
) | ||
|
||
var _ = Describe("AgentConfig delete", func() { | ||
Context("when an existing AgentConfig is delete", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context("when an existing AgentConfig is delete", func() { | |
Context("when an existing AgentConfig is deleted", func() { |
default-secrets: "kubernetes-secrets" | ||
verbosity: "debug" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops! Thanks for fixing that 👍
Signed-off-by: Yingrong Zhao <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Ship it 🚀
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: Yingrong Zhao <[email protected]>
This PR implements the ability to allow users to configure plugins they need in agent-config resource.
Current workflow:
It creates a new agent config controller to manage the lifecycle of the plugin installation. The controller dispatch jobs to the porter agent to install plugins onto a persistent volume through a pvc (temporary persistent volume claim) managed by the agent config controller. Once the installation is done, the controller sets labels, including all installed plugins and the agent config name, on the persistent volume. Then the controller will create a new pvc. The new pvc will use the hash of all the plugins information as its name and the plain text version of all the plugin information as its label.
When a new agent action is created, it will query using the plugin hash derived from its agent config and mount a plugin pvc using that name.
If no existing plugin pvc exits, the agent action will wait for the pvc to be created before kubernetes schedules it.
When a plugin volumes is no longer needed, the current approach is to allow k8s GC to do the work for us. All the operator is doing when a agent config is deleted is to make sure to remove itself from the pvc's and pv's
OwnerReferences
value. Once no other resource is referencing a volume, GC will know to delete it permanently.To make sure we are not deleting any volumes that still could being used, when a volume is being reused by multiple agent config, the controller will make sure to add all agent config into the owner reference before marking the agent config status has complete.
Couple issues that may need to be worked out before this feature is complete:
- Since currently, the operator doesn't have the capability to clean up pods after each job has been finished, the above cleanup workflow for plugin volumes only removes the deleted agent config from the plugin volume's owner reference list. we need to validate that when all the pods are deleted, the plugin volumes will be deleted too. Delete temporary resources when agent completes #54
Resolution: I would like to work on this as a separate PR and just have the current deletion behavior as only removing
OwnerReference
.- Another issue that the current code has not solved is how to install multiple porter plugins with one agent job. From my discovery, porter currently does not seem to allow installation for multiple plugins with one command. Ability to install multiple plugins defined in one agent config #123
Resolution: This will be a separate PR in porter to implement this new feature. For now, only
kubernetes
plugins will be installed. Users will only able to definekubernetes
plugin and configure its version and feedURL- We probably should set default plugin configuration in both the installer bundle and the controller code. If someone were to delete the default agent config defined by the installer bundle, the controller can still set a default kubernetes pluin in the code
- We need to add integration tests for delete behavior
- This feature is also missing document. For example, where all the default values for plugin configs are set Add document for plugin install feature #122