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

Helm chart deployment - some issues and confusing operatorless options #17

Closed
calexandre opened this issue Jul 25, 2022 · 9 comments
Closed

Comments

@calexandre
Copy link

Hey!
First of all, great solution you have here...

I've been trying to deploy this using the helm chart provided, and I'm finding that some of the values are confusing...
For example, the operatorless property, is somehow confusing...because if I leave the operatorless to its default (which is false), the deployment fails due to a missing volume mount; after digging into the chart, I found out the operatorless mode is dependent on another deployment (i think it is the gateway one).

In the end, I was not able to deploy the chart and I had to use the manifests and deploy them via kubectl.

Another thing I found out, is that you are creating the namespace on the chart...this renders multiple problems when the namespace already exists... Perhaps you could consider using the builtin helm options to automatically handle the namespace creation instead of declaring and creating it on the chart?

Thank you!

@nmate
Copy link
Contributor

nmate commented Jul 25, 2022

Hi!

Thanks for the comment and the valuable ideas! Let us first resolve the problem with helm install. There have been changes to stunner helm recently, and we decided to move the code under the l7mp/stunner-helm repo (from l7mp/stunner). I gave a quick try to the most recent version
helm install stunner stunner/stunner --set stunner.operatorless.enabled=true
and did not experience any failure. Could you please share the error/output you get?

Thanks!

@calexandre
Copy link
Author

calexandre commented Jul 25, 2022

The error only happened with helm install stunner stunner/stunner --set stunner.operatorless.enabled=false.
With operatorless.enabled=true the issue was with helm itself, where it complained about the namespace already existing (I used a different namespace).

Can you try your command while specifying a different namespace aswell?

At the moment I don't have a shell available, so I cannot write you the exact error, but I will once I get home.

@davidkornel
Copy link
Member

Hi @calexandre!
I'll try to help you with your issues. First of all operatorless will be renamed to standalone. but what this really means?
When deploying (helm install stunner stunner/stunner --set stunner.operatorless.enabled=true) in standalone(or operatorless for now) mode all the resources (such as LB services, NetworkPolicies) will be installed manually by Helm. They will be static, which you might modify by hand.
By deploying with the gateway operator (helm install stunner stunner/stunner --set stunner.operatorless.enabled=false) all the moving parts will be handled on the fly by the gateway operator. You might be interested in this setup since it is the primary "version".

So to resolve your issue I think you might have forgotten to install the operator itself. You can do it with the following command: helm install stunner-gateway-operator stunner/stunner-gateway-operator.

the deployment fails due to a missing volume mount;

This is because, without the gateway operator, the essential volume(stunnerd-configmap) will be missing. The operator creates it based on the following manifests: gatewayconfig and gatewayclass.
This is the reason why the gatewayoperator must be installed first for now(will remove this necessity).

Have you done these commands in the right order?

helm repo add stunner https://l7mp.io/stunner
helm repo update

helm install stunner-gateway-operator stunner/stunner-gateway-operator

helm install stunner stunner/stunner

Another thing I found out, is that you are creating the namespace on the chart...this renders multiple problems when the namespace already exists... Perhaps you could consider using the builtin helm options to automatically handle the namespace creation instead of declaring and creating it on the chart?

And at last thanks for pointing out the namespace issue. I will fix it ASAP. Thanks for the advice.

I'll be back with new docs and with the refactored helm charts if they are done.

@davidkornel
Copy link
Member

@calexandre I brought the new helm versions for you.
The way of deploying the chart has changed a bit, so the commands are not quite right in my last comment. But the reasons to your issues still stand.
Here you can find the updated docs.

helm install stunner-gateway-operator stunner/stunner-gateway-operator --create-namespace --namespace=<your-namespace>
helm install stunner stunner/stunner --create-namespace --namespace=<your-namespace>

Please install both charts in the same namespace for now.

@calexandre
Copy link
Author

Thank you!
I will try what you suggested and report back!

Regarding the gateway operator, perhaps you could consider merging it with the stunner chart instead of deploying and managing two separate charts...

@calexandre
Copy link
Author

calexandre commented Aug 2, 2022

@davidkornel I installed both charts successfully following your instructions...
I then tried to follow your cloudretro example scrupulously, but the gateway was never in the READY state. Due to this I was never able to test the deployment properly because the last script fails...

I installed both stunner and stunner-gateway-operator in the default namespace.

Am I missing something here?

This is the status of the gateway:

$ kubectl describe gateway udp-gateway

Name:         udp-gateway
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  gateway.networking.k8s.io/v1alpha2
Kind:         Gateway
Metadata:
  Creation Timestamp:  2022-08-02T16:24:41Z
  Generation:          1
  Managed Fields:
    API Version:  gateway.networking.k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:gatewayClassName:
        f:listeners:
          .:
          k:{"name":"udp-listener"}:
            .:
            f:allowedRoutes:
              .:
              f:namespaces:
                .:
                f:from:
            f:name:
            f:port:
            f:protocol:
    Manager:         kubectl-client-side-apply
    Operation:       Update
    Time:            2022-08-02T16:24:41Z
  Resource Version:  218353
  UID:               2fc48de7-7e2c-4442-93e0-3d98dd270f53
Spec:
  Gateway Class Name:  stunner-gatewayclass
  Listeners:
    Allowed Routes:
      Namespaces:
        From:  Same
    Name:      udp-listener
    Port:      3478
    Protocol:  UDP
Status:
  Conditions:
    Last Transition Time:  1970-01-01T00:00:00Z
    Message:               Waiting for controller
    Reason:                NotReconciled
    Status:                Unknown
    Type:                  Scheduled
Events:                    <none>

@calexandre
Copy link
Author

calexandre commented Aug 2, 2022

I overcome the issue by installing the gateway class specified in the stunner-gateway-operator instructions :)
I somehow missed that part because the cloudretro example doesn't mention that on the stunner installation section - perhaps the example documentation can be improved by mentioning that both GatewayClass and GatewayConfig are required.

Edit: Can you consider shipping the default GatewayClass and GatewayConfig manifests with the stunner-gateway-operator chart? Since it is only installed once per stunner deployment, it seems logical to include those in the chart.

So, everything appears to be working as expected!

Thank you!

@bbalint105
Copy link
Contributor

bbalint105 commented Aug 3, 2022

Sorry, my bad.
I haven't been updating the demo doc with the new helm-install.

In the old one they are, but in the new one, GatewayClass and GatewayConfig are not included, so users can customize them themselves.
Although I think I will still provide default ones in the cloudretro demo.

I will fix it ASAP.

@rg0now rg0now added the bug label Aug 31, 2022
@rg0now
Copy link
Member

rg0now commented Aug 31, 2022

Dear @calexandre, I'm going to close this issue assuming that we covered the problems you encountered, Feel free to reopen if you have any further questions

@rg0now rg0now closed this as completed Aug 31, 2022
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

No branches or pull requests

5 participants