-
Notifications
You must be signed in to change notification settings - Fork 499
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
tests: refactor fault trigger #896
Conversation
StartVM(*VM) error | ||
} | ||
|
||
type VirshVMManager struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a new file for VirshVMManager, e.g. vm_virsh.go and a new file for VirshVMManager, e.g. vm_qm.go what do you think?
@@ -225,3 +129,81 @@ func stripEmpty(data string) string { | |||
} | |||
return strings.Join(stripLines, "\n") | |||
} | |||
|
|||
type QMVMManager struct { | |||
*sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this RWMutex
is not used.
@@ -43,7 +45,7 @@ func main() { | |||
logs.InitLogs() | |||
defer logs.FlushLogs() | |||
|
|||
mgr := manager.NewManager() | |||
mgr := manager.NewManager(vmManager) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better not to allow invalid --vm-manager
value, we can initialize vm manager and handle errors here, e.g.
var vmMgr VMManager
if vmManager == "qm" {
vmMgr = NewQMManager()
} else if vmManager == "virsh" {
vmMgr = NewVirshManager()
} else {
// fatal error
}
mgr := manager.NewManager(vmMgr)
if some users configured an invalid value, but our program still works, this will confuse people because they don't know what virtual manager we use from the command-line flags unless they know implementation details
/run-e2e-in-kind |
} else if vmManagerName == "virsh" { | ||
vmManager = &VirshVMManager{} | ||
} else { | ||
slack.NotifyAndPanic(fmt.Errorf("stability test have not supported the vm manager:[%s],please choose [qm] or [virsh].", vmManagerName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fault-trigger, slack.Webhook endpoint is not provided. IMO a simple fatal error is enough.
/run-e2e-in-kind |
this lgtm but I cannot test it locally |
@cofyc this pr has been tested pass in stability env. now it is still running in stability env. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Have you run this PR on the |
it has run in stability env for serval days,except the apiserver fault trigger case. |
@weekface it has not run in stability env which use |
You can run it at 149 env. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-e2e-in-kind |
@xiaojingchen CI failed |
/run-e2e-in-kind |
…db-operator into agent-support-qm
/run-e2e-in-kind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@cofyc PTAL again |
(cherry picked from commit 4d2dd0c)
What problem does this PR solve?
qm
as vm managerWhat is changed and how does it work?
Add a new VM manager to support qm.
Change the stability config, add the map of VM name and VM IP, avoid fault-trigger agent dynamic acquisition of IP
new stability config file:
Check List
Tests
Code changes
Side effects
Related changes
Does this PR introduce a user-facing change?: