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

Add reset button to reset all resources and scheduler configuration #26

Closed
sanposhiho opened this issue Sep 23, 2021 · 22 comments · Fixed by #105 or #114
Closed

Add reset button to reset all resources and scheduler configuration #26

sanposhiho opened this issue Sep 23, 2021 · 22 comments · Fixed by #105 or #114
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@sanposhiho
Copy link
Member

sanposhiho commented Sep 23, 2021

Add reset button

  • delete all resources
  • change scheduler configuration to default one.

need to change both frontend and backend.

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 23, 2021
@sanposhiho sanposhiho changed the title Add reset button to reset all resources Add reset button to reset all resources and scheduler configuration Sep 23, 2021
@sivchari
Copy link
Member

/assign

@196Ikuchil
Copy link
Contributor

Hi, @sivchari
I'm came from #67
So is this what you're saying?
We need to implement one service that has a function(method) to receive a Reset request and call "DeleteAll" method of each service. Or, implement it on a simulator service that already exists.

As one additional solution, I would like to propose that integrating with this #64 functions. These are similar to yours in terms of the scope of service its uses and manages.
(and just as time, I'm having trouble deciding on this package name. )
what do you think?
Cc @sanposhiho

@sanposhiho
Copy link
Member Author

As I said before, we had two ways for this feature.

  1. create one API to reset all resources and scheduler config.
  2. create multiple APIs to reset each resource and scheduler config.

And @sivchari has already created new API to reset scheduler in #54.
This means we take the way 2.

And, #68 creates "delete all" method in each service. So, in backend, all that remain tasks for us to do is to improve handler layers for each resource.

@196Ikuchil

As one additional solution, I would like to propose that integrating with this #64 functions.

I cannot catch what your "integrating" means.
At least, I don't think it is a good idea to implement this reset feature in the same package as import/export feature package in #64. If we do so, that package will be too big.

@196Ikuchil
Copy link
Contributor

196Ikuchil commented Dec 20, 2021

Oops, it's "in each service". I was mistaken and off the main purpose, sorry...

@sanposhiho

I cannot catch what your "integrating" means.

This is means I just thought they were similar in that it calls each service if we had taken the way 1.

@sanposhiho
Copy link
Member Author

Yes, if we had taken the way 1, we would have created a new service to delete all resources.

@sivchari
Copy link
Member

Sorry, I was not clear enough.

We need to implement one service that has a function(method) to receive a Reset request and call "DeleteAll" method of each service.

I was thinking of implementing something like this.
This is because if we want to call each handler, we need to call each request one at a time.
I mentioned this question because I thought this was not so good and it would be better to create a new handler and call DeleteAll for each service in one request.

/cc @sanposhiho
/cc @196Ikuchil

@sanposhiho
Copy link
Member Author

sanposhiho commented Dec 21, 2021

@sivchari
So, do you want to delete API to reset scheduler configuration created in #54?

@sivchari
Copy link
Member

sivchari commented Dec 21, 2021

@sanposhiho

I think #54 is API that only restart scheduler.
That's why, I suggest following way.

  1. Define new API to call DeleteAll method each service has.
  2. Call API to restart scheduler configuration and API which Step 1 created to delete all resources.

What do you think ?

@sanposhiho
Copy link
Member Author

sanposhiho commented Dec 21, 2021

Okay. So you want to create two APIs:

  • the one is to delete all resources.
  • the another is to reset scheduler configuration.

Why do you think it is better to split into these two APIs? (instead of creating one API to delete all resources and reset scheduler.)

@sivchari
Copy link
Member

We defined endpoint to reset scheduler configuration /api/v1/schedulerconfiguration now.
This endpoint means to update something to scheduler configration.
It doesn't contain meaning to delete each service.
So, I think it is better to split into these two APIs.

If this solution isn't better, I think we must rethink endpoint.
(e.g. DELETE /resources)

@sanposhiho
Copy link
Member Author

sanposhiho commented Dec 21, 2021

So, if we take your suggest way (= creating two endpoints), where should the API which resets all resources be located?

@sanposhiho
Copy link
Member Author

sanposhiho commented Dec 21, 2021

What I want to say is:
If we do it your way, I think we cannot define an appropriate endpoint path for the API which resets all resources.
DELETE /resources is not a good idea. If we want to create it, we need to move ALL resource related paths under /resources. (like: /api/v1/resources/pods/:name) I feel it's little hard to do that only for this reason.

Given the current design of the API path, I think these are only two ways we can go I said first.

  1. create one API to reset all resources and scheduler config.
  2. create multiple APIs to reset each resource and scheduler config.

If we take the way 1, we can create new paths for each resources like PUT /api/v1/pods/reset, PUT /api/v1/nodes/reset... etc.
And if we take the way 2 (one API to delete all resources and reset scheduler), we can create a new path like /api/v1/reset.

@sivchari
Copy link
Member

I haven't catch your idea, yet. #70 is designed by way 2 ? It looks like way 1 design, because this fix means to delete only scheduler configuration. BTW, way 2 is finally called each endpoint. I don't prefer to take that. If we take way 1, client call API one time. So, I prefer to take way 1. I think way 1 endpoint path is probably that you said (/api/v1/reset). I agree with your idea and I think this endpoint path is good to delete all resources and scheduler configuration.
What do you think ?

@sanposhiho
Copy link
Member Author

Note that I never said which way I think is best for this feature. I just asked you.

So, do you prefer the way 1? I guess your answer is "yes". Because you said ↓:

way 2 is finally called each endpoint. I don't prefer to take that

And please answer this previous question. I guess your answer for this question should be "yes".

So, here are your remain tasks for this feature, right?:

  1. delete API to reset scheduler configuration created in add reset scheduler configuration #54.
  2. create new service to reset all resources and scheduler configuration.
  3. create one API to reset all resources and scheduler configuration with the service layer created in 2.
  4. improve frontend

If your idea is different from the above, please share your design idea that you think is the best in detail.


As I mentioned in the PR description, #70 is not needed in case that we go the way 1. So, I'll close this after confirming that you agree to the above.

This PR is not needed in case that we re-design #26 to create one API to reset all resources and scheduler config.
So, let me /hold this.
#70

@sivchari
Copy link
Member

Note that I never said which way I think is best for this feature. I just asked you.

I know.

do you prefer the way 1?

Yes, your predict is correct.

So, here are your remain tasks for this feature, right?:

  1. delete API to reset scheduler configuration created in add reset scheduler configuration add reset scheduler configuration #54.
  2. create new service to reset all resources and scheduler configuration.
  3. create one API to reset all resources and scheduler configuration with the service layer created in 2.
  4. improve frontend

I agree with it.

@sanposhiho
Copy link
Member Author

Okay. We've finally got an agreement. Go ahead @sivchari :)

@sanposhiho
Copy link
Member Author

I closed #70

@sivchari
Copy link
Member

I hold this until #68 is merged

@196Ikuchil
Copy link
Contributor

Hi @sivchari
How is the progress of the webUI implementation?
I will also implement a import/export button on PR #64 , soon later.

I'd like to put it around here. ↓ But I worry about any conflicts.
CC. @sanposhiho
screenshot

@sivchari
Copy link
Member

Hi @196Ikuchil, sorry to respond late.

We haven't complete all API to delete all resources and scheduler configuration, so I'm not doing implementation webUI yet.

I'd like to put it around here. ↓ But I worry about any conflicts.

As above reason, you don't worry about any conflicts.

Thanks.

@sanposhiho
Copy link
Member Author

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Feb 14, 2022
@k8s-ci-robot
Copy link
Contributor

@sanposhiho: Reopened this issue.

In response to this:

/reopen

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
4 participants