Add virtxml helper and internal status puller#128
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors how VM/domain resource “status” (e.g., CPU, Memory) is retrieved by introducing an internal status abstraction backed by either libvirt calls (active domains) or XML parsing (inactive domains), and adds a small virtXML helper + fetcher layer to support that.
Changes:
- Refactors domain status retrieval to
internal/statusand switches callers to amap[vmtypes.SourceType]intrequest/response shape. - Adds
pkg/virtXML/fetcherto parse domain XML and extract requested fields (CPU/Memory). - Removes the legacy
DomCon/domain_statuspackage.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/virtXML/virtxml.go | Changes ConvertExistingDomain to accept a no-arg XML getter closure. |
| pkg/virtXML/fetcher/domain.go | New XML fetcher that parses domain XML and fills requested source values. |
| pkg/virtXML/fetcher/fetcher_test.go | Unit tests for XML fetcher behavior (CPU/Memory/unknown/error). |
| pkg/types/source.go | Introduces vmtypes.SourceType and constants (CPU/Memory). |
| internal/status/datadog.go | New DataDog interface + New(isActive) selector. |
| internal/status/domain.go | Domain interface used by status implementations (GetMaxVcpus/GetXMLDesc). |
| internal/status/libvirt.go | Active-domain status retrieval via libvirt calls. |
| internal/status/xml.go | Inactive-domain status retrieval via XML fetcher. |
| internal/status/status_test.go | Unit tests covering New(), libvirt-based CPU, and XML-based CPU. |
| api/Control.go | Updates API handlers to request CPU via map[vmtypes.SourceType]int and use typed results. |
| DomCon/domain_list.go | Switches to new internal/status retrieval and updated UpdateFromDomain signature. |
| DomCon/domainList_status/updator.go | Refactors to typed status map + dom.IsActive() based selection. |
| DomCon/domainList_status/updator_test.go | New tests for UpdateFromDomain behavior for active/inactive + error path. |
| DomCon/domain_status/xml_unparse.go | Removes legacy XML unparse helper. |
| DomCon/domain_status/status.go | Removes legacy status types/interfaces. |
| DomCon/domain_status/newStatus.go | Removes legacy status retrieval implementations. |
| DomCon/domain_status/cpu.go | Removes legacy CPU retrieval helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ConvertExistingDomain takes functions as domain.GetXMLDesc or any string based function. | ||
| // It should be wrapped with function which returns string and error qualified with libvirt-xml.Domain format. | ||
| func ConvertExistingDomain(getXMLDesc func() (string, error)) (*libvirtxml.Domain, error) { | ||
| xmlStr, err := getXMLDesc() | ||
| if err != nil { |
There was a problem hiding this comment.
ConvertExistingDomain signature was changed to accept only a no-arg getXMLDesc func(), but the package still has tests (pkg/virtXML/virtxml_test.go) calling the old signature with flags (ConvertExistingDomain(0, func(DomainXMLFlags)...)). This will break the build; update those tests/callers or add a small backward-compatible wrapper that accepts flags and forwards via closure.
pkg/virtXML/fetcher/domain.go
Outdated
| for k := range sources { | ||
| switch k { | ||
| case vmtypes.CPU: | ||
| sources[vmtypes.CPU] = int(domainXML.VCPU.Value) | ||
| case vmtypes.Memory: | ||
| sources[vmtypes.Memory] = int(domainXML.Memory.Value) | ||
| default: | ||
| return nil, fmt.Errorf("unknown source type: %s", string(k)) | ||
| } |
There was a problem hiding this comment.
Unknown source handling is inconsistent between implementations: XMLFetcher returns an error for an unknown SourceType, while LibvirtStatus only logs a warning and continues. With the shared DataDog interface, this means active vs inactive domains will behave differently for the same input. Align behavior (either both error, or both ignore/leave untouched) so callers don't need state-specific error handling.
DomCon/domain_list.go
Outdated
| go func(targetDom libvirt.Domain) { | ||
| defer wg.Done() | ||
| if err := DC.DomainListStatus.UpdateFromDomain(dataDog, &targetDom, state, []domainStatus.SourceType{domainStatus.CPU}, logger); err != nil { | ||
| sources := map[vmtypes.SourceType]int{vmtypes.CPU: 0} | ||
| if err := DC.DomainListStatus.UpdateFromDomain(dataDog, &targetDom, isActive, sources, logger); err != nil { | ||
| logger.Sugar().Errorf("Failed to retrieve status for domain UUID=%s: %v", uuid, err) | ||
| } |
There was a problem hiding this comment.
The goroutine closes over the loop variable uuid from the outer loop. This can log the wrong UUID (and any future logic using uuid would be wrong) because the loop continues before the goroutine runs. Pass uuid as an explicit parameter to the goroutine (or assign to a local variable inside the loop) alongside dom.
kwonkwonn
force-pushed
the
migrate_domain_status_puller
branch
from
515b96e to
3d3a0c8
Compare
kwonkwonn
force-pushed
the
migrate_domain_status_puller
branch
from
3d3a0c8 to
6c50a9a
Compare
|
마이그레이션 목적은 다음과 같습니다.
이를 통해 호출자는 인스턴스의 on/off와 관계없이 상태를 가져올 수 있습니다. 단점: |
Type of Change
Summary
virtXML 를 보강하고, XML에 /fetcher를 추가했습니다.
특정값을 읽어오고 업데이트하는 역할을 수행할거에용
Related Issue
Closes #131
Test Plan
make testpasses