Use "proper" web framework for HTTP servers #13
Labels
a/tech_debt
Area: related to tech debt
c/autoscaling/scheduler
Component: autoscaling: k8s scheduler
Comments
bayandin
pushed a commit
that referenced
this issue
Feb 23, 2023
The current solution has some rough edges, but I'm not familiar enough with the Docker go api to sort them out yet. For now, this only works if you (at least) specify the image tag explicitly - e.g. "vmdata-temp:latest" instead of "vmdata-temp". This commit also adds --pull to force-pull the source image, bypassing this mechanism.
bayandin
pushed a commit
that referenced
this issue
Mar 22, 2023
The current solution has some rough edges, but I'm not familiar enough with the Docker go api to sort them out yet. For now, this only works if you (at least) specify the image tag explicitly - e.g. "vmdata-temp:latest" instead of "vmdata-temp". This commit also adds --pull to force-pull the source image, bypassing this mechanism.
bayandin
pushed a commit
that referenced
this issue
Mar 22, 2023
The current solution has some rough edges, but I'm not familiar enough with the Docker go api to sort them out yet. For now, this only works if you (at least) specify the image tag explicitly - e.g. "vmdata-temp:latest" instead of "vmdata-temp". This commit also adds --pull to force-pull the source image, bypassing this mechanism.
sharnoff
changed the title
sharnoff
changed the title
sharnoff
added a commit
that referenced
this issue
Feb 2, 2024
Closes #760. AFAIK this hasn't been an issue in the past, but as we're trying to improve reliability, it's good to get this out of the way before it becomes an issue. Note that this PR is quite minimal - expanding the existing tech debt we have around how the scheduler plugin handles HTTP requests. It's probably ok *enough* for now. I don't expect we'll be making too many changes to it in the near future. See also: #13. Tested locally by forcing it to panic on every request: diff --git a/pkg/plugin/run.go b/pkg/plugin/run.go index 007554a..6da7728 100644 --- a/pkg/plugin/run.go +++ b/pkg/plugin/run.go @@ -262,8 +262,10 @@ func (e *AutoscaleEnforcer) handleAgentRequest( } } - pod.vm.mostRecentComputeUnit = &e.state.conf.ComputeUnit - return &resp, 200, nil + panic(errors.New("test panic!")) + + // pod.vm.mostRecentComputeUnit = &e.state.conf.ComputeUnit + // return &resp, 200, nil } // getComputeUnitForResponse tries to return compute unit that the agent supports The change appears to work as intended.
sharnoff
added a commit
that referenced
this issue
Feb 12, 2024
Closes #760. AFAIK this hasn't been an issue in the past, but as we're trying to improve reliability, it's good to get this out of the way before it becomes an issue. Note that this PR is quite minimal - expanding the existing tech debt we have around how the scheduler plugin handles HTTP requests. It's probably ok *enough* for now. I don't expect we'll be making too many changes to it in the near future. See also: #13. Tested locally by forcing it to panic on every request: diff --git a/pkg/plugin/run.go b/pkg/plugin/run.go index 007554a..6da7728 100644 --- a/pkg/plugin/run.go +++ b/pkg/plugin/run.go @@ -262,8 +262,10 @@ func (e *AutoscaleEnforcer) handleAgentRequest( } } - pod.vm.mostRecentComputeUnit = &e.state.conf.ComputeUnit - return &resp, 200, nil + panic(errors.New("test panic!")) + + // pod.vm.mostRecentComputeUnit = &e.state.conf.ComputeUnit + // return &resp, 200, nil } // getComputeUnitForResponse tries to return compute unit that the agent supports The change appears to work as intended.
|
We are okay with the current state, the code is very readable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See: #8 (comment)
There's a few places we have HTTP servers:
(*AutoscaleEnforcer) runPermitHandlerpkg/agent/serve.go:startInformantServercmd/informant/main.go:main(*AutoscaleEnforcer) startDumpStateServer(*agentState) StartDumpStateServer(time of writing: 2023-01-10, commit 65d1528)
(updated: 2023-10-12, commit 166c8d0)
Ideally we'd have something less home-grown here, to reduce custom code & leverage existing wisdom.
The text was updated successfully, but these errors were encountered: