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

Create networking-console-plugin.md #1645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

upalatucci
Copy link

open again #1606 pr

Copy link
Contributor

openshift-ci bot commented Jun 17, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign danwinship for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jupierce
Copy link
Contributor

/retest

Copy link
Contributor

@tssurya tssurya left a comment

Choose a reason for hiding this comment

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

Is this enhancement complete and ready for reviews? Many sections are empty, wondering if that was intentional


At present, several pages within the networking section of our console are defined within the console repository.
However, working with the console repository has slowed down development and there are multiple motivations to convert static console plugins into dynamic ones described [here](https://github.com/spadgett/enhancements/blob/master/enhancements/console/dynamic-plugins.md#motivation).
To address this, we propose relocating the relevant code to a separate repository named "networking-console-plugin" and integrating it into the cluster-networking-operator.
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a stupid question, but why specifically do we think this should live in CNO and not in https://github.com/kubevirt/cluster-network-addons-operator ?
what would be the plugin install's impact on resource consumption on the overall ocp cluster?
when we put this in CNO its going to be ON by default; are we sure we want console on all variants of sno,ocp,hcp,microshift etc?

Copy link
Author

Choose a reason for hiding this comment

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

Hei @tssurya. Right now the Networking console section is integrated with the console and in order to don't change the user experience, with this upgrade we would like to have this section delivered by default with openshift anyways.

So if this addons-operator is delivered by default and it's not 'optional' I would leave the decision to the CNO team.

Copy link
Contributor

@tssurya tssurya Jul 11, 2024

Choose a reason for hiding this comment

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

CNO team and adds-on team are separate, my concern is around enabling yet another daemonset lifecycle management from CNO -> resources wise what's the increase/impact here?
infact I am not sure I am convinced networking console is a CNO thing.. perhaps it should stay in console? -> the reasoning provided is a "process reason -> let's move faster?" what is the user/functional impact/motivation here?
how does the upgrade design look like where things are currently part of console but now moving to CNO?
@upalatucci : thanks for taking the time to answer my questions; please add all this information into the enhancement so that the right context is being set (I think all the sections in the enhancements should be filled or a reasoning must be provided as to why its not relevant.).

Copy link
Author

@upalatucci upalatucci Jul 11, 2024

Choose a reason for hiding this comment

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

The Console team is stretched with a lot of work and no one gets to maintain and develop new features for network UI for years so we're taking over this part.

For us taking over means making the code future-proof and scalable fitting our standards.
We have already received several requests but cannot keep adding things in an already busy and old console code.

Copy link
Author

@upalatucci upalatucci Jul 11, 2024

Choose a reason for hiding this comment

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

when we put this in CNO its going to be ON by default; are we sure we want console on all variants of sno,ocp,hcp,microshift etc?

Yes sure. We want this enabled in all places. So that when they access (or add in the future) the console, they can see all the Networking pages like Routes, Services, NADS and everything they already see now.

Copy link
Author

@upalatucci upalatucci Jul 11, 2024

Choose a reason for hiding this comment

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

what would be the plugin install's impact on resource consumption on the overall ocp cluster?

The CNO needs to handle 4 resources:

  • deployment (to deploy the docker image with static js/css/svg/jpg files)
  • configmap (nginx configuration)
  • service (expose the static files)
  • consoleplugin (to let the console load the static files)

(I'll add those details in the doc)

Copy link
Member

Choose a reason for hiding this comment

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

@upalatucci thanks for summary.
From what I read we have three choices how to move forward.

  1. Place the plugin directly into CNO repo (plugin will be enabled and available by default), where Ugo's team will claim ownership over the plugin code.
  2. Place the plugin in the CNaddonsO repo (plugin will need to be enabled manually, since the addons need to be installed manually)
  3. Splitting the logic, by placing the plugin into a separate repo and source it by the CNO or CNaddosO, based on the default behaviour thats expected (similar to how the Virtualization Operator is delivering the kubevirt-plugin).

Here I would go with the third option, so the ownership is clearly defined.
I would suggest to tak to the kubevirt folks and check how thier operator is vendoring the plugin.


### User Stories

N/A. For the end user the UI will be the same. The cluster admin will have the option to disable and enable the plugin but it's not relevant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
N/A. For the end user the UI will be the same. The cluster admin will have the option to disable and enable the plugin but it's not relevant.
N/A. For the end user the UI will be the same. The cluster admin will have the option to disable and enable the plugin but it's not relevant.

I'm interested in this disable option, can you please expand more?

Copy link
Author

Choose a reason for hiding this comment

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

All the plugins installed in the cluster can be listed using this command

oc get consoleplugins

You can verify which plugins are enabled using this command

oc get console.operator.openshift.io cluster -o jsonpath='{.spec.plugins}'

Disabling plugins means that the pages and features they provide will not be shown in the console.

For example, this plugin defined an entire section in the console called 'Networking'. Disabling this plugin means that the user can no longer see this section in the console UI.


### Goals:

Enhanced Development Efficiency: By isolating networking console pages into a dedicated repository, developers can expedite development cycles and foster faster iterations, ultimately enhancing overall development efficiency.
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 enhancement around have a new repo in OCP or integrating that with CNO or both?

Copy link
Author

Choose a reason for hiding this comment

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

This is the repo: https://github.com/openshift/networking-console-plugin

In the CNO repo we need just to add the logic to install the plugin and handle some needed resources.
CNO pr to integrate the plugin is here: openshift/cluster-network-operator#2322


Move Networking Console Pages (Done): Transfer relevant code files and resources from the console repository to the "networking-console-plugin" repository and refactor them. Ensure accurate migration of dependencies and configurations to maintain seamless functionality.

Integration with Cluster-Networking-Operator: Modify the cluster-networking-operator to incorporate the networking console plugin as an operand. Update configurations and references accordingly to seamlessly integrate the networking pages with the operator.
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this specifically entail? the workflow description seems empty?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Essentially, the integration of the networking-console-plugin in CNO entails creating and reconciling a few resources, and patching the consoles.operator.openshift.io resource to enable it.

This is very similar to what was done in cluster-monitoring-operator to add the monitoring-plugin by default (the Observe section in the console).


### Dev Preview -> Tech Preview

N/A
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 dev preview aimed behind a feature gate?

Copy link
Author

Choose a reason for hiding this comment

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

No there is no feature gate

Copy link
Contributor

Choose a reason for hiding this comment

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

we cannot merge features without feature gate in ocp.. please plan and add it in your enhancement

Copy link
Author

Choose a reason for hiding this comment

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

We as UI team do not add ocp feautres. We just create UI to use them. We are moving part of the code and refactoring it. For the first version, the user will not even notice the difference.
The features that I'm mentioning in this document are 'UI features' like 'clicking this button you'll create a Route'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Routes are part of ingress operator, so shouldn't that piece go there? cc @Miciah

Copy link
Author

@upalatucci upalatucci Jul 25, 2024

Choose a reason for hiding this comment

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

I don't want to split this section into such small pieces. It's not the best choice to move faster in the future. Creating a plugin repo with it's configuration and so on, is not that simple.
I had to request a brew repo, prow configuration, jira configuration, the ART team to review everything. Several things do not justify pieces so small.
We already talk with the SVN team and they agreed to have this into their operator...

Copy link
Author

Choose a reason for hiding this comment

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

the plugin can even hide some pages if some functionalities are not installed in the cluster.

For example if ingress operator is not installed we can hide routes if you think is the best solution.
But keep in mind that right now the user can see those pages either way

## Summary

At present, several pages within the networking section of our console are defined within the console repository.
However, working with the console repository has slowed down development and there are multiple motivations to convert static console plugins into dynamic ones described [here](https://github.com/spadgett/enhancements/blob/master/enhancements/console/dynamic-plugins.md#motivation).
Copy link
Contributor

@tssurya tssurya Jul 10, 2024

Choose a reason for hiding this comment

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

I am not a UI expert so could you please explain what is the difference between a static plugin and dynamic plugin from console perspective?
Also when I read the provided link it states: "Static plugins can only be updated on the OpenShift release cadence. We want to enable plugin teams to move faster."
CNO also is the same.. we put in new features on release cadence, maybe I am missing something?

Copy link
Author

@upalatucci upalatucci Jul 11, 2024

Choose a reason for hiding this comment

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

this is the console repo: https://github.com/openshift/console
In this repo you can find:

  • the main code that handle the console structure and provide an SDK for plugins
  • static plugins that using the SDK can the integrated into the console and add pages and functionality (example)

Static plugins are built within the console.

Dynamic plugins instead are defined in separate repo (example) where they can define their workflow, dependenices and move faster in general. Those plugins CAN be built with a separate lifecycle and needs to be integrated by other operators to be part of openshift.
The operator has to handle a couple of resources to make this work.

The main benefit is not what you are mentioning but this:

We already have a half-dozen teams contributing to console. We need to be able to scale as more teams contribute. This will be difficult if everyone is contributing to a single monorepo.

Our experience with kubevirt-plugin proved that this works and we now move faster than ever.

We are excited to do the same with networking, take responsibility of this section and add so much features

Copy link
Member

Choose a reason for hiding this comment

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

Our experience with kubevirt-plugin proved that this works and we now move faster than ever.

Yes, this is the way how the console plugins should be delivered, by the matching operator. Console itself can be hosting any more of plugins, matter of fact is that we are gradually try to move the plugins, both static and dynamic, from our repo because of various reasons, some of them also mentioned here.

@upalatucci upalatucci force-pushed the patch-1 branch 2 times, most recently from 558b59d to 4ca9e52 Compare July 11, 2024 09:33
Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

Thanks @upalatucci for the proposal

@@ -0,0 +1,154 @@
---
title: networking-console-plugin
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: networking-console-plugin
title:console-networking-plugin

Copy link
Author

Choose a reason for hiding this comment

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

I'm afraid to start a new naming convention here. Multiple plugins end with console-plugin

## Summary

At present, several pages within the networking section of our console are defined within the console repository.
However, working with the console repository has slowed down development and there are multiple motivations to convert static console plugins into dynamic ones described [here](https://github.com/spadgett/enhancements/blob/master/enhancements/console/dynamic-plugins.md#motivation).
Copy link
Member

Choose a reason for hiding this comment

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

Our experience with kubevirt-plugin proved that this works and we now move faster than ever.

Yes, this is the way how the console plugins should be delivered, by the matching operator. Console itself can be hosting any more of plugins, matter of fact is that we are gradually try to move the plugins, both static and dynamic, from our repo because of various reasons, some of them also mentioned here.


At present, several pages within the networking section of our console are defined within the console repository.
However, working with the console repository has slowed down development and there are multiple motivations to convert static console plugins into dynamic ones described [here](https://github.com/spadgett/enhancements/blob/master/enhancements/console/dynamic-plugins.md#motivation).
To address this, we propose relocating the relevant code to a separate repository named "networking-console-plugin" and integrating it into the cluster-networking-operator.
Copy link
Member

Choose a reason for hiding this comment

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

@upalatucci thanks for summary.
From what I read we have three choices how to move forward.

  1. Place the plugin directly into CNO repo (plugin will be enabled and available by default), where Ugo's team will claim ownership over the plugin code.
  2. Place the plugin in the CNaddonsO repo (plugin will need to be enabled manually, since the addons need to be installed manually)
  3. Splitting the logic, by placing the plugin into a separate repo and source it by the CNO or CNaddosO, based on the default behaviour thats expected (similar to how the Virtualization Operator is delivering the kubevirt-plugin).

Here I would go with the third option, so the ownership is clearly defined.
I would suggest to tak to the kubevirt folks and check how thier operator is vendoring the plugin.

@JacobTanenbaum
Copy link

@upalatucci Could you add to this doc which team is responsible for maintaining the networking-console-plugin?

@upalatucci
Copy link
Author

@JacobTanenbaum yes sure. I reinforce here and I'll do it in the doc that our team is responsible for this plugin @tnisan

Copy link
Contributor

openshift-ci bot commented Aug 2, 2024

@upalatucci: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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.

None yet

6 participants