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

Multi-Context Support #4218

Merged

Conversation

tangledbytes
Copy link
Member

@tangledbytes tangledbytes commented Sep 20, 2021

Description

This PR adds support for multiple contexts in meshery. This is a critical PR, has a huge surface area for bugs to pop up and also changes the way sections of meshery server interact with kubernetes cluster.

Details:

  1. This PR changes the "unit" of cluster communication from a kubeconfig file to per-user per-cluster kubernetes contexts. Following are a few results of such changes:
    • Any number of kubconfigs can be uploaded to meshery server.
    • Each user gets to have any number of contexts for a meshery instance
    • "Current context" for each user can differ.
    • Long running processes will not be impacted with future context changes of the user.
    • Potential issues (that the author can/could think of):
      • What happens when there is no context?
        • In-cluster deployments - A context is created from the mounted secrets
        • No kubeconfig - no context is created and no handler is created. This does not stops the http flow though, each handler can choose to accept or reject the request. This behaviour was chosen so as to keep meshery working even if no kubernetes cluster is reachable.
      • What if a current-context already exists and a new kubeconfig is uploaded with another "current-context"?
        • Depends on the provider. At the moment, both None as well as Meshery Cloud will ignore the "current-context" claim of the new context. If the user wants to switch the context, they can do so via the API/UI.
      • What if a "current-context" is deleted? Will there be a new current-context chosen?
        • No
      • Who can see my contexts?
        • Anyone who has access to the meshery instance.
      • Who can modify my contexts?
        • No one. Contexts are immutable.They can be deleted but only by the "owner" of the context.
      • What happens if two users invoke the same API with different contexts?
        • Both of their execution will be isolated. There are no global handlers in meshery server.
      • What happens if two users invoke different Meshery APIs under the same context?
        • Meshery operations remain isolated.
      • What happens if I delete all of my contexts?
        • You are back to the initial stage. Meshery will try to read kubeconfig and find your current-context.
        • Other users will not see those contexts anymore.
        • Inflight operations (yours or those of other users) proceed unaffected and conclude as requested.
      • What happens if the owner of the context has their user account deleted?
        • Context remains. Created_By value goes to nil.
  2. Tests done:
    • On local system:
      • With one cluster in kubeconfig:
        • Startup meshery
        • Ping cluster
        • Check details of context in settings
        • Deploy meshery operator
        • Delete current context
        • Deploy service mesh (interact with adapter)
        • Run performance test
      • With two clusters in kubeconfig:
        • Startup meshery
        • Ping each cluster
        • Change context from the UI
        • Check details of context in the settings
        • Delete current context
        • Delete all contexts
        • Deploy meshery operator
        • Deploy service mesh (interact with adapter)
        • Run performance test

Notes for Reviewers
Although this PR is functional (to the best of my knowledge) but still it should not be merged. Some cleanup items are left to do like:

  1. Use meshkit errors

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
…nd dummy implementation in remote provider

Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
…a in http request contexts

Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2021

Codecov Report

Merging #4218 (9ab1e2d) into master (22906f4) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4218   +/-   ##
=======================================
  Coverage   32.12%   32.12%           
=======================================
  Files          62       62           
  Lines        5252     5252           
=======================================
  Hits         1687     1687           
  Misses       3140     3140           
  Partials      425      425           
Flag Coverage Δ
gointegrationtests 10.75% <ø> (ø)
unittests 24.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfb387d...9ab1e2d. Read the comment docs.

@tangledbytes
Copy link
Member Author

Reviewers,
@sudo-NithishKarthik, this PR effects UI. Some changes has been done while some still needs to be done. Kindly take a look at it.
@navendu-pottekkat, @piyushsingariya this PR changes the behaviour of some APIs and removes some. This may have implications on mesheryctl. Please look out for them and lemme know.
@bariqhibat this PR changes the way kubehandlers are provided to graphql. Please look out and lemme know if it causes some issues.

@pottekkat
Copy link
Contributor

Thanks for the note @Utkarsh-pro Will test out mesheryctl and see if there are any issues.

Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: leecalcote <leecalcote@gmail.com>
@leecalcote
Copy link
Member

leecalcote commented Jan 4, 2022

Syncing with master.

@Utkarsh-pro this PR does not include the Kubernetes context selector, does it?

Screen Shot 2022-01-04 at 2 08 01 AM

Without a selector, users will be unable to identify which context a given operation is to be sent to.

@leecalcote
Copy link
Member

@Utkarsh-pro when multiple contexts are connected, only a single cluster chip shows on the Dashboard. We're expecting to show all connected contexts here, yeah?

Screen.Recording.2022-01-04.at.2.17.44.AM.mov

@leecalcote
Copy link
Member

@Utkarsh-pro I'm getting:

INFO[0042] attempting to save kubernetes context to remote provider 
DEBU[0042] saving context to remote provider - constructed URL: https://meshery.layer5.io/user/contexts 
WARN[0042] failed to save the context: Status Code: 201.failed to save kubernetes context 

I'll send you the full logs...

@tangledbytes
Copy link
Member Author

Syncing with master.

@Utkarsh-pro this PR does not include the Kubernetes context selector, does it?

Screen Shot 2022-01-04 at 2 08 01 AM

Without a selector, users will be unable to identify which context a given operation is to be sent to.

@leecalcote, yes this PR doesn't have the mentioned context selector. Right now the only way to switch contexts is to do it from the settings (by selecting the context from the dropdown).

UI certainly needs to be improved, it could be a part of this PR or could be brought in another PR considering that the UX doesn't changes even if we bring in this PR.

@tangledbytes
Copy link
Member Author

@Utkarsh-pro when multiple contexts are connected, only a single cluster chip shows on the Dashboard. We're expecting to show all connected contexts here, yeah?

Screen.Recording.2022-01-04.at.2.17.44.AM.mov

Yes, that's right. We should be showing the same here as well.

@tangledbytes
Copy link
Member Author

@Utkarsh-pro I'm getting:

INFO[0042] attempting to save kubernetes context to remote provider 
DEBU[0042] saving context to remote provider - constructed URL: https://meshery.layer5.io/user/contexts 
WARN[0042] failed to save the context: Status Code: 201.failed to save kubernetes context 

I'll send you the full logs...

Ah, I think this is just a minor bug. I think the status code that the remote provider sending is 201 (resource created) while the server is expecting 200. So the API is working just fine, just need to change the expected status code.

Will fix this 👍

Copy link
Contributor

@piyushsingariya piyushsingariya left a comment

Choose a reason for hiding this comment

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

Things that failed during testing in my system

  • Failed to delete current-context
  • Failed to delete all context
  • Failed to deploy operator in a non-current-context cluster
  • If I upload the same kubeconfig twice, multiple entries for contexts get saved
2022-01-15.15-13-28.mp4

@leecalcote
Copy link
Member

@Utkarsh-pro thoughts on @piyushsingariya's feedback?

Heads-up on merge conflicts.

@tangledbytes
Copy link
Member Author

These are concerning findings @piyushsingariya. I have been away from this PR for a while now but will look into these behaviours and will confirm if these are indeed reproducible bugs and were not left out for certain reasons.

I should be able to get back on this within a few hours.

// @leecalcote

@tangledbytes
Copy link
Member Author

Also, thank you much @piyushsingariya for testing this out understanding you may had to go out of your way to test this PR out 🙁 .

@leecalcote
Copy link
Member

leecalcote commented Jan 20, 2022

@Utkarsh-pro here's the design for the Kubernetes context switcher - https://www.figma.com/file/SMP3zxOjZztdOLtgN4dS2W/Meshery-UI?node-id=3984%3A21705

Screen Shot 2022-01-20 at 2 22 34 PM

@leecalcote
Copy link
Member

@Utkarsh-pro any comment here? Something that you can implement?

@leecalcote
Copy link
Member

A few merge conflicts have popped up. FYI, @Utkarsh-pro.

@tangledbytes
Copy link
Member Author

@leecalcote, yes I can work on the the given UI. Have been lagging quite a lot but will make sure that this PR goes through ✅ .

@leecalcote
Copy link
Member

@Utkarsh-pro sounds great. Can you demo progress at this week's Meshery development meeting?

@leecalcote
Copy link
Member

@Utkarsh-pro, how about this week? Can you demo progress at this week's Meshery development meeting?

@tangledbytes
Copy link
Member Author

Hey @leecalcote, I have conflicting meeting on Wednesday. I'll try to show the progress on Friday's call though.

@leecalcote
Copy link
Member

@Utkarsh-pro, excellent! // @debo19

@leecalcote
Copy link
Member

Tomorrow is the big day! :)

Revolyssup and others added 5 commits February 11, 2022 15:32
Signed-off-by: ashish <ashishjaitiwari15112000@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
@tangledbytes
Copy link
Member Author

tangledbytes commented Feb 13, 2022

@leecalcote all the changes that I had locally are here now. I have also resolved the merge conflicts.

Screenshots
Screenshot 2022-02-13 at 6 09 41 PM

Screenshot 2022-02-13 at 6 09 55 PM

@piyushsingariya I could not replicate duplicate upload issue, I am not sure what might have caused that 🤔 . To answer other questions:

  • Users can delete their "active context" but doing so will have not net effect as the reload will re-create the context, that's why it seems you cannot delete the context.
  • Operator deployment in multiple cluster is not yet part of this PR. The reason is mostly because there is no design for that yet. Right now operator is deployed as soon as meshery comes up, that cannot be done if the contexts themselves are closely tied to users. We basically need design here, just the deployment part - the rest of the things are in place to support multiple operators and data sources.

// @sudo-NithishKarthik @Revolyssup

Signed-off-by: ashish <ashishjaitiwari15112000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants