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

Inject NSM-envs into NSM-containers #18

Closed
wants to merge 1 commit into from

Conversation

glazychev-art
Copy link
Contributor

@glazychev-art glazychev-art commented Jul 27, 2021

networkservicemesh/deployments-k8s#2009

Sometimes we want to use additional NSM envs for NSC (e.g. NSM_REQUEST_TIMEOUT).
If the user set these envs, webhook should inject them into NSM-containers.

Signed-off-by: Artem Glazychev artem.glazychev@xored.com

Signed-off-by: Artem Glazychev <artem.glazychev@xored.com>
@edwarnicke
Copy link
Member

edwarnicke commented Jul 27, 2021

@glazychev-art What problem are we trying to solve here? I'm a little reluctant to have the mechanism for tuning these parameters be scraping them from other containers configs.

Why do we need to be tuning these parameters?

@glazychev-art
Copy link
Contributor Author

@edwarnicke
I ran into this problem when using annotations for kernel clients in integration-tests.
For example, here:

cat > patch-nsc.yaml <<EOF
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: nsc-kernel
spec:
  template:
    spec:
      nodeName: $NSC_NODE
      containers:
        - name: nsc
          env:
            - name: NSM_NETWORK_SERVICES
              value: kernel://autoscale-icmp-responder/nsm-1
            - name: NSM_REQUEST_TIMEOUT
              value: 30s
EOF

I try to add annotations:

...
      annotations:
        networkservicemesh.io: kernel://autoscale-icmp-responder/nsm-1
...

NSM_NETWORK_SERVICES - we will take from annotations.
But we also need to use NSM_REQUEST_TIMEOUT.
How we can add this env to nsc?

@edwarnicke
Copy link
Member

Why do we need to use NSM_REQUEST_TIMEOUT? What is the default?

@glazychev-art
Copy link
Contributor Author

Default is 15s.
Apparently for some tests this is not enough

@edwarnicke
Copy link
Member

@glazychev-art OK... why is that not enough for some tests?

@glazychev-art glazychev-art marked this pull request as draft July 28, 2021 11:30
@glazychev-art
Copy link
Contributor Author

@edwarnicke
So, for example, we have nse-composition tests. Here we need to wait until a few passthrough endpoints and nse are ready.
And, what if 15s is not enough for some users? How they can set up this timeout?
Also, nsc uses other envs - DialTimeout, MaxTokenLifetime and so on. Should the user be able to set these values?

@glazychev-art
Copy link
Contributor Author

@denis-tingaikin
cc

@glazychev-art
Copy link
Contributor Author

@edwarnicke
There is a problem with setting NSM config variables passed by user.
We have the following options:

  1. Use NSM envs that are passed by user and inject them into NSM-containers (current PR)
  2. Expand the current networkservicemesh.io annotation and add new parameters. For example:
...
      annotations:
        networkservicemesh.io: kernel://icmp-responder/nsm-1, request-timeout=...
... 
  1. Your option

Please share your thoughts

@edwarnicke
Copy link
Member

So, for example, we have nse-composition tests. Here we need to wait until a few passthrough endpoints and nse are ready.

Why is that taking 15s?

@glazychev-art
Copy link
Contributor Author

Because the client is waiting for its request to go through all of the passthrough-nses sequentially.

We can also consider new option:
4. Improve default parameters to cover all requirements. (e.g. increase the default RequestTimeout)

@edwarnicke
Copy link
Member

@artem-belov How many passthrough NSEs? How long is each taking? 15s is a very long time...

@glazychev-art
Copy link
Contributor Author

glazychev-art commented Aug 13, 2021

Our test uses 4 passthrough NSEs

@glazychev-art
Copy link
Contributor Author

Each NSE starts asynchronously in a random order. And in the worst case, it may take more than 15s.

As for the possible options, I suppose expanding annotations is better

@glazychev-art
Copy link
Contributor Author

Re-test nse-composition - networkservicemesh/deployments-k8s#2690

@glazychev-art
Copy link
Contributor Author

the root issue was closed: networkservicemesh/deployments-k8s#2009

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