Handle status monitor container reload #45

dteplits · 2022-12-08T09:11:10Z

Flow description:

When status monitor container is loaded, it creates a zone file and initializes it with the header and A records. In this case SOA serial initial value will be 1
Each time status monitor updates zone file, it increments SOA serial value
Auto plugin inside Corefile is responsible to reload zone file newest data into the DNS when it identifies that SOA serial value was changed

PR adds the below logic:

If status monitor container was reloaded, on its startup it should check whether zone file already exist.
If yes, it should continue the existing SOA serial sequence instead of setting it back to 1.
The reason for that is the below edge case:

If zone file already exist and holds SOA serial = 1 (for example) and status monitor container was restarted
Then even after status monitor will override zone file content with the new data, the SOA serial will remain the same (=1)
Thus it will not be reloaded by the auto plugin since the SOA serial didn't change

Release note:

NONE

oshoval

Thanks
partial review

Please consider updating PR desc with more precise flow that cause the dead end

oshoval · 2022-12-08T09:23:43Z

pkg/zonemgr/zone_file_cache.go

@@ -55,10 +56,33 @@ func (zoneFileCache *ZoneFileCache) prepare() {
 	zoneFileCache.generateHeaderSuffix()
 	zoneFileCache.soaSerial = 0


please read the file here, and set the serial either 0 or whatever the file has if exists
then no need this part
https://github.com/kubevirt/kubesecondarydns/pull/45/files#diff-91aeb36dc70c24eda218702f6964e34e7cbb3c7f29cce4f45ed3b09b6f59cb11R44

I have each file responsible for a different kind of activity, ZoneFile is responsible for dealing with the disk zone file and ZoneFileCache is responsible for zone cache maintenance. And ZoneManager is responsible for the flow logic. I don't think mixing it is a good idea.

It is fine that there will be an helper function that reads the file in order to determine the SOA to start from.
It is better imo than to create the header twice

oshoval · 2022-12-08T09:24:11Z

pkg/zonemgr/zone_file_cache.go

+func fetchSoaSerial(content []byte) int {
+	contentStr := string(content)
+	var ind1, ind2 int
+	if ind1 = strings.Index(contentStr, "("); ind1 == 0 {
+		return 0
+	}
+	if ind2 = strings.Index(contentStr[ind1:], " "); ind2 == 0 {
+		return 0
+	}
+	soaSerial := strings.TrimSpace(contentStr[ind1+1 : ind1+ind2])
+	if soaSerialInt, err := strconv.Atoi(soaSerial); err != nil {
+		return 0
+	} else {
+		return soaSerialInt
+	}
+}
+


there might be a way using regex to fetch it, will be nice if we can check please

Btw according https://www.ietf.org/rfc/rfc1035.txt you can add comments (start with ;)
if the first line will be a comment with the serial, you can read just that line instead the whole file and get the serial

Changed to using regex.
Regarding the "reading line instead of the whole file", I can add it in a different PR

oshoval · 2022-12-08T12:12:52Z

pkg/zonemgr/zone_manager.go

@@ -40,6 +40,10 @@ func (zoneMgr *ZoneManager) prepare() {
 	zoneMgr.zoneFileCache = NewZoneFileCache(nameServerIP, domain)

 	zoneMgr.zoneFile = NewZoneFile(zoneFileName + zoneMgr.zoneFileCache.domain)
+
+	if content, _ := zoneMgr.zoneFile.readFile(); content != nil && len(content) > 0 {


please verify that error is nil as well
if it is !=nil it is allowed to only be "not found"

oshoval · 2022-12-11T08:27:16Z

pkg/zonemgr/zone_file_cache.go

+func fetchSoaSerial(content []byte) int {
+	if result := soaSerialReg.Find(content); result != nil {
+		soaSerial := strings.TrimSpace(string(result)[1:])
+		if soaSerialInt, err := strconv.Atoi(soaSerial); err == nil {
+			return soaSerialInt
+		} else {
+			return 0
+		}
+	} else {
+		return 0
+	}
+}
+


Consider please something like this

package main import ( "fmt" "regexp" ) func main() { s := "/user/999/edit" re := regexp.MustCompile(`/user/([0-9]+)/edit$`) x := re.FindStringSubmatch(s) fmt.Println(x[1]) }

Prints the 999

I'm doing pretty match the same:
your example:
re := regexp.MustCompile(/user/([0-9]+)/edit$)
x := re.FindStringSubmatch(s)
my code:
soaSerialReg = regexp.MustCompile("\([0-9]+ ")
result := soaSerialReg.Find(content)

In this case regexp.Find does the job, I'm not sure why we need to use FindStringSubmatch() function.
The rest of the code removes unnecessary white spaces (if any) and converts string into a numeric.
So I'm not sure what exactly can/should be improved here.

Its saves usage of TrimSpace and Atoi, we don't need to reinvent please, when higher level exists.
you can give a regex, and it will do everything else for you, giving you the chunk by its own
just need to make sure please you still handle errors as well if the file is corrupted / NA etc

EDIT - right the Atoi is not saved, but it use FindStringSubmatch instead of Find

AlonaKaplan

General comment that I had to give on previous PRs (and can be fixed on a follow-up PR). I would change the code so there will be a clear API between ZoneFile, ZoneFileCache and Zonemanager.
I mean, treat them as if they are in separate packages and there is no access to prived methods and data members.
I believe it will make the code much more readable and safe.

AlonaKaplan · 2022-12-12T06:43:19Z

pkg/zonemgr/zone_file.go

@@ -19,3 +17,7 @@ func NewZoneFile(fileName string) *ZoneFile {
 func (zoneFile *ZoneFile) writeFile(content string) (err error) {
 	return os.WriteFile(zoneFile.zoneFileFullName, []byte(content), zoneFilePerm)
 }
+
+func readFile(zoneFileName string) ([]byte, error) {


Why isn't it a method of ZoneFile like writeFile?

Because it's a "util" function, I use it inside ZoneFileCache in order to fetch SoaSerial.
In the previous version this method was part of ZoneFile and ZoneManager did the job since it holds ZoneFile instance.
But Or suggested to move this logic into ZoneFileCache file and use readFile as a helper function.
Here is the conversation:
#45 (comment)

I can create a util file for readFile function, I just wanted to spare creating another file just for one method that anyway matches ZoneFile functionality

You can strive to make it as an API or so please (as Alona suggested #45 (review))

I prefer the previous design. I find it cleaner that the ZoneCache is not familiar with the ZoneFile.

ZoneFile is a general util / this is the purpose of it, so it is totally fine imo,
there should not be two places that determine the serial and rebuild the header
the flow was much harder to grasp
maybe we can fine tune to something that we all agree upon

Why not reading the SOA from the file via ZoneManager and passing it to the ZoneCache?

This is an option, this is not the previous design
as long as we set initial serial once and build the header once without rebuild in case a serial exists, it is good imo

The problem is with the zone file name. It is initiated based on the domain name. Which is set inside the ZoneCache. So in current design you can't read the SOA from the file and pass it to ZoneCache without first initializing the ZoneCache. Perhaps it can be changed by changing the design slightly, not sure it's worth it

You can find the only file in the folder, if exist by scanning the folder
if we all like the idea
then you don't need the name of it
if you want to bullet proof it, you can find the only file that starts with db in the folder etc

Imo the current status is better, or at least worth to think later about something but not as part of this PR
unless you insist

another option is to read ad hoc the ORIGIN in order to just know the zone file name

I have a problem in general with approaching ZoneFile from ZoneFileCache as it breaks current design. I will soon push another PR with a suggestion of small design change to handle it better. Let's see if it solves the issue.

AlonaKaplan · 2022-12-12T06:44:16Z

pkg/zonemgr/zone_file_cache.go

 	zoneFileCache.header = zoneFileCache.generateHeader()
-	zoneFileCache.content = zoneFileCache.header


Why was this line removed and he is it related to this PR?

I just noticed that it's unnecessary. Not related to this PR.

Now it's back?

Previously in this PR zoneFileCache.content = zoneFileCache.header was removed. Now it's back, why?

AlonaKaplan · 2022-12-12T06:53:56Z

pkg/zonemgr/zone_file_cache.go

@@ -23,6 +25,8 @@ const (
 	adminEmailDefault = "email"
 )

+var soaSerialReg = regexp.MustCompile("\\([0-9]+ ")


I would change to "SOA .*\(([0-9]+) " to be more explicit.

AlonaKaplan · 2022-12-12T07:02:18Z

pkg/zonemgr/zone_file_cache_test.go

+				Expect(os.RemoveAll("zones")).To(Succeed())
+			})
+
+			It("should set SOA serial equal to existing value", func() {


I think the method the has to be tested is the prive one NewZoneFileCache and not an internal method.
I would call NewZoneFileCacheand check the header. Same as the other tests.

Doesn't seem this comment was handled.

dteplits · 2022-12-14T09:30:33Z

General comment that I had to give on previous PRs (and can be fixed on a follow-up PR). I would change the code so there will be a clear API between ZoneFile, ZoneFileCache and Zonemanager. I mean, treat them as if they are in separate packages and there is no access to prived methods and data members. I believe it will make the code much more readable and safe.

Methods that I consider as a public API are:
ZoneManager:

NewZoneManager()
UpdateZone()
ZoneFileCache:
NewZoneFileCache()
updateVMIRecords()
ZoneFile:
NewZoneFile()
writeFile()
readFile() - "static" function

If I want to treat them as they are in a separate packages, I should make them public.
It's not really necessary since it's still the same package but for the clarity of the code perhaps I should.
Regarding the unit tests, I will change it to use public methods only.

oshoval · 2022-12-14T09:31:53Z

Regarding the unit tests, I will change it to use public methods only.

Unit test can check private methods as well

dteplits · 2022-12-14T09:49:34Z

Regarding the unit tests, I will change it to use public methods only.

Unit test can check private methods as well

Then I don't see a problem :)
The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test
Anyway, I can use a public one instead if necessary.

oshoval · 2022-12-14T09:53:19Z

Regarding the unit tests, I will change it to use public methods only.

Unit test can check private methods as well

Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.

The problem that Alona raise is the decoupling between the components IIUC
unrelated to my specific comment about unit tests.
Connection between components is done by the APIs.

dteplits · 2022-12-14T09:58:10Z

Regarding the unit tests, I will change it to use public methods only.

Unit test can check private methods as well

Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.

The problem that Alona raise is the decoupling between the components IIUC unrelated to my specific comment about unit tests. Connection between components is done by the APIs.

So based on the list I specified earlier (public API methods), converting those methods to public should do the job?
This is an API as I define it. Do I miss something?

AlonaKaplan · 2022-12-15T11:39:42Z

Regarding the unit tests, I will change it to use public methods only.

Unit test can check private methods as well

Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.

The problem that Alona raise is the decoupling between the components IIUC unrelated to my specific comment about unit tests. Connection between components is done by the APIs.

So based on the list I specified earlier (public API methods), converting those methods to public should do the job? This is an API as I define it. Do I miss something?

Moving zone_file.go and zone_file_cache.go to package internal should block accessing their public methods outside of the package - https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface

oshoval · 2022-12-15T11:51:46Z

Moving zone_file.go and zone_file_cache.go to package internal should block accessing their public methods outside of the package - https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface

Imo we can just use Cap and non Cap
draw 3 boxes, one for each module, and just the functions that have cross talk declare as API and expose.
The internal can be the next step if needed, and then it might be easier because each module will also have its own folder
(as the internal is based on folder structure)

oshoval · 2022-12-18T11:57:14Z

pkg/zonemgr/zone_file_cache.go

+	if result := soaSerialReg.FindStringSubmatch(content); result != nil && len(result) > 0 {
+		soaSerial := result[1]
+		if soaSerialInt, err := strconv.Atoi(soaSerial); err == nil {
+			return soaSerialInt
+		} else {
+			return 0
+		}
+	} else {
+		return 0
+	}


Lets try please to reduce else and indent

serial := 0 if result := soaSerialReg.FindStringSubmatch(content); result != nil && len(result) > 0 { soaSerial := result[1] if soaSerialInt, err := strconv.Atoi(soaSerial); err == nil { serial = soaSerialInt // or even just return soaSerialInt, and then in the last line return 0 instead } } return serial

same can be done for the function above please

dteplits · 2022-12-18T12:34:21Z

Regarding the unit tests, I will change it to use public methods only.

Unit test can check private methods as well

Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.

The problem that Alona raise is the decoupling between the components IIUC unrelated to my specific comment about unit tests. Connection between components is done by the APIs.

So based on the list I specified earlier (public API methods), converting those methods to public should do the job? This is an API as I define it. Do I miss something?

Moving zone_file.go and zone_file_cache.go to package internal should block accessing their public methods outside of the package - https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface

will be fixed in a separate PR

AlonaKaplan · 2022-12-20T13:08:58Z

pkg/zonemgr/zone_manager.go


-	zoneMgr.zoneFile = NewZoneFile(zoneFileName + zoneMgr.zoneFileCache.domain)
+func (zoneMgr *ZoneManager) initSoaSerial() int {


I think this method should be moved to ZoneFile. I would call it ReadSoaSerial. If the file doesn't exist it should return nil, otherwise, the value from the file.

AlonaKaplan · 2022-12-20T13:09:04Z

pkg/zonemgr/zone_manager.go

+	return serial
+}
+
+func fetchSoaSerial(content string) int {


AlonaKaplan · 2022-12-21T08:48:05Z

pkg/zonemgr/zone_file_test.go

 			testFileFunc(zoneFileUpdatedContent)
 		})
+
+		It("should read SOA serial", func() {


Please add a test testing ReadSoaSerial to the scenario the file doesn't exist.

AlonaKaplan · 2022-12-21T08:50:46Z

pkg/zonemgr/zone_file.go

+}
+
+func (zoneFile *ZoneFile) ReadSoaSerial() int {
+	serial := 0


I would return nil in case the file doesn't exist. The logic to set 0 in cases there is no value in the existing file should reside in the ZoneCache.

Imo it is not needed, we don't need to differentiate and have logic for both nil and 0
just 0 is enough
it makes the code less nice and less readable with all those *int, conversions and such
the name of the function is ReadSoaSerial its already high level, it should be allowed to return 0 as a serial

I don't agree. ReadSoaSerial should only read the value from the file, it shouldn't have any heuristic of calculating the value in case the file doesn't exist, this logic should reside in the ZoneCache like the rest of the logic.

It is like a default value

you can name the function with something like CalculateSoaSerial
and then it will be responsible to give either 0 or nil,
otherwise we are drifting with conversions and ptr that we can and should eliminate

i wont insist, but it is not correct imo, keep it simple is much better
maybe there is a middle way you can think of that will make it nicer and you will like ?

btw there is one solution for it for example:
instead getting the DOMAIN by the ZoneManager, get it whenever needed,
this way we might able to pass the result less between modules

AlonaKaplan · 2022-12-21T08:52:17Z

pkg/zonemgr/zone_file.go

+
+func (zoneFile *ZoneFile) ReadSoaSerial() int {
+	serial := 0
+	if content, err := zoneFile.readFile(); err == nil && content != nil {


We shouldn't ignore errors. If the error is that the file doesn't exist, it is ok, and we should return nil soaSerial. But for any other error we should return an error and not proceed as if nothing happened.

AlonaKaplan · 2022-12-21T08:56:58Z

pkg/zonemgr/zone_file_cache_test.go

@@ -160,7 +159,7 @@ var _ = Describe("cached zone file content maintenance", func() {

 		When("interfaces records list contains multiple vmis", func() {
 			BeforeEach(func() {
-				zoneFileCache = NewZoneFileCache(nameServerIP, domain)
+				zoneFileCache = NewZoneFileCache(nameServerIP, domain, 0)


Please add a test with non zero value.

AlonaKaplan · 2022-12-21T11:53:32Z

pkg/zonemgr/zone_file.go

+	return &isExist, err
+}
+
+func (zoneFile *ZoneFile) ReadSoaSerial() (*int, error) {


import "k8s.io/apimachinery/pkg/api/errors"

content, err := zoneFile.readFile() if err != nil && errors.IsNotFound(err) { return nil, nil } if err != nil || content == nil { return nil, err } return fetchSoaSerial(string(content))

I'm not sure that os.ReadFile() returns a specific error in case of FileNotFound. The documentation does not specify it. I only found a recommendation to check it with os.Stat()

AlonaKaplan · 2022-12-21T11:56:01Z

pkg/zonemgr/zone_file_cache.go

 	zoneFileCache.header = zoneFileCache.generateHeader()
-	zoneFileCache.content = zoneFileCache.header


Previously in this PR zoneFileCache.content = zoneFileCache.header was removed. Now it's back, why?

AlonaKaplan · 2022-12-21T11:57:39Z

pkg/zonemgr/zone_manager.go

-	zoneMgr.zoneFileCache = NewZoneFileCache(nameServerIP, domain)
-
-	zoneMgr.zoneFile = NewZoneFile(zoneFileName + zoneMgr.zoneFileCache.domain)
+	if soaSerial, err := zoneMgr.zoneFile.ReadSoaSerial(); err != nil {


I find this more readable.

soaSerial, err := zoneMgr.zoneFile.ReadSoaSerial() if err != nil { return err } zoneMgr.zoneFileCache = NewZoneFileCache(nameServerIP, domain, soaSerial) return nil

AlonaKaplan · 2022-12-21T12:04:39Z

pkg/zonemgr/zone_file_cache_test.go

@@ -111,7 +117,7 @@ var _ = Describe("cached zone file content maintenance", func() {

 		When("interfaces records list contains single vmi", func() {
 			BeforeEach(func() {
-				zoneFileCache = NewZoneFileCache(nameServerIP, domain)
+				zoneFileCache = NewZoneFileCache(nameServerIP, domain, nil)


I would change at least one on the next tests to use non nil soa number.

I'll add a new test for it

oshoval

Thanks

oshoval · 2022-12-21T12:00:20Z

pkg/zonemgr/zone_file.go

+	} else if errors.Is(err, os.ErrNotExist) {
+		err = nil
+	}
+	return &isExist, err


why we need *bool and not just bool ?

oshoval · 2022-12-21T12:00:46Z

pkg/zonemgr/zone_file.go

+	if isFileExist, err := zoneFile.isFileExist(); !*isFileExist || err != nil {
+		return nil, err
+	}
+	if content, err := zoneFile.readFile(); content == nil || err != nil {


worth to add please the logic of isFileExist to readFile
wdyt ?

I considered it and preferred to separate them as 1. each function has a different responsibility 2. more readable and clear

I understand, but on the other hand one can argue readFile should handle non existing file

in this case it returns content=nil, just like os.ReadFile()

if readFile would check before that isFileExist it will know to return content == nil if file doesn't exists, with err == nil, and different error in case it couldn't read the file contents

I won't insist on it, but imo it is nicer

I would leave it as it is if you don't mind, I find it more readable

oshoval · 2022-12-21T12:02:07Z

pkg/zonemgr/zone_file.go

+	}
+}
+
+func fetchSoaSerial(content string) (*int, error) {


why we need *int not just int ?

please use uint32

soaSerialInt can be named soaSerial

since I have to be able to return nil

just return 0, why we need nil ?

#45 (comment)

oshoval · 2022-12-21T12:05:37Z

pkg/zonemgr/zone_file_cache.go

@@ -40,10 +39,16 @@ type ZoneFileCache struct {
 	vmiRecordsMap map[string][]string
 }

-func NewZoneFileCache(nameServerIP string, domain string) *ZoneFileCache {
+func NewZoneFileCache(nameServerIP string, domain string, soaSerial *int) *ZoneFileCache {


why *int and not just int ?

since I want to support nil value

just return 0, why we need nil ?
(same for all places)

#45 (comment)

oshoval · 2022-12-21T12:06:37Z

pkg/zonemgr/zone_file_cache.go

@@ -40,10 +39,16 @@ type ZoneFileCache struct {
 	vmiRecordsMap map[string][]string
 }

-func NewZoneFileCache(nameServerIP string, domain string) *ZoneFileCache {
+func NewZoneFileCache(nameServerIP string, domain string, soaSerial *int) *ZoneFileCache {
+	soaSerialInt := 0


soaSerialInt better to just use the argument soaSerial as is, and it will have (does it already ? if not please check) the value zero if the fetch didnt find a previous serial

the idea is to handle nil value if ZoneFile not exist

but 0 is good enough, as long as there is no error, we don't need to differentiate between nil and 0

#45 (comment)

oshoval · 2022-12-21T12:09:32Z

pkg/zonemgr/zone_manager.go

-	zoneMgr.zoneFile = NewZoneFile(zoneFileName + zoneMgr.zoneFileCache.domain)
+	if soaSerial, err := zoneMgr.zoneFile.ReadSoaSerial(); err != nil {
+		return err
+	} else {


the else can be dropped, please simply do the code in it, because if the "if" caught it had a return

changed following #45 (comment)

oshoval · 2022-12-21T12:10:12Z

pkg/zonemgr/zone_manager_test.go

-		zoneMgr = NewZoneManager()
+		var err error
+		zoneMgr, err = NewZoneManager()
+		Expect(err).To(BeNil())


ToNot(HaveOccurred())

oshoval · 2022-12-21T12:10:23Z

pkg/zonemgr/zone_file_test.go

+		It("should read SOA serial", func() {
+			Expect(os.WriteFile(zoneFileName, []byte(headerSoaSerial), 0777)).To(Succeed())
+			soaSerial, err := zoneFile.ReadSoaSerial()
+			Expect(err).To(BeNil())


ToNot(HaveOccurred())

When zone manager is created, if zone file already exist and contains SOA serial, then the next SOA serial should continue the existing one. Signed-off-by: Diana Teplits <dteplits@redhat.com>

AlonaKaplan · 2022-12-21T18:03:50Z

/approve

oshoval

Thanks

oshoval · 2022-12-25T08:32:11Z

pkg/zonemgr/zone_file_test.go

+
+		It("should return nil when try to read SOA serial", func() {
+			soaSerial, err := zoneFile.ReadSoaSerial()
+			Expect(err).To(BeNil())


Please consider fixing this one on either this PR or a follow one
Expect(err).ToNot(HaveOccurred())

Signed-off-by: Diana Teplits <dteplits@redhat.com>

AlonaKaplan · 2022-12-25T15:24:11Z

/approve

kubevirt-bot · 2022-12-25T15:24:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlonaKaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AlonaKaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

oshoval · 2023-01-29T07:33:40Z

I think we didn't cover the following case:
Status-monitor container crashed, for example if someone removed kubevirt for more than 2 minutes
and then deleted all the VMs
Now reinstall kubevirt, and the container is starting with the previous SOA, but it doesnt write the file
as long as there is no update, and the zone file still have the old VMs
the safest solution is to rewrite the file (with serial++) without vms if the soa was there (but with the header and such, so we wont lose the serial in case of another restart)

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 8, 2022

kubevirt-bot requested review from AlonaKaplan and oshoval December 8, 2022 09:11

kubevirt-bot added the size/M label Dec 8, 2022

oshoval reviewed Dec 8, 2022

View reviewed changes

dteplits force-pushed the dns_sec branch 2 times, most recently from d3ec9aa to 3348fc4 Compare December 8, 2022 14:23

dteplits marked this pull request as draft December 8, 2022 15:32

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2022

dteplits force-pushed the dns_sec branch 2 times, most recently from ea7c401 to 75b2815 Compare December 8, 2022 15:57

dteplits marked this pull request as ready for review December 8, 2022 16:06

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2022

kubevirt-bot requested a review from oshoval December 8, 2022 16:06

oshoval reviewed Dec 11, 2022

View reviewed changes

AlonaKaplan reviewed Dec 12, 2022

View reviewed changes

dteplits force-pushed the dns_sec branch from fcea751 to 6452ce7 Compare December 18, 2022 10:51

oshoval reviewed Dec 18, 2022

View reviewed changes

dteplits force-pushed the dns_sec branch from 6452ce7 to 69c2ad9 Compare December 18, 2022 12:00

dteplits force-pushed the dns_sec branch from 69c2ad9 to ea1ec2b Compare December 18, 2022 13:15

oshoval mentioned this pull request Dec 19, 2022

Zone manager package refactoring #51

Merged

dteplits force-pushed the dns_sec branch from ea1ec2b to 6a4ca63 Compare December 20, 2022 12:45

AlonaKaplan reviewed Dec 20, 2022

View reviewed changes

dteplits force-pushed the dns_sec branch from 6a4ca63 to 6b05ba3 Compare December 21, 2022 07:51

AlonaKaplan reviewed Dec 21, 2022

View reviewed changes

dteplits force-pushed the dns_sec branch from 6b05ba3 to 3f93d37 Compare December 21, 2022 11:20

kubevirt-bot added size/L and removed size/M labels Dec 21, 2022

AlonaKaplan reviewed Dec 21, 2022

View reviewed changes

oshoval reviewed Dec 21, 2022

View reviewed changes

zone manager: Handle exisitng SOA serial use case

26a3f6c

When zone manager is created, if zone file already exist and contains SOA serial, then the next SOA serial should continue the existing one. Signed-off-by: Diana Teplits <dteplits@redhat.com>

dteplits force-pushed the dns_sec branch from 3f93d37 to b4f977b Compare December 21, 2022 15:26

oshoval approved these changes Dec 25, 2022

View reviewed changes

kubevirt-bot assigned oshoval Dec 25, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 25, 2022

oshoval reviewed Dec 25, 2022

View reviewed changes

tests, zone manager: Adding unit test for existing SOA serial use case

f04e901

Signed-off-by: Diana Teplits <dteplits@redhat.com>

dteplits force-pushed the dns_sec branch from b4f977b to f04e901 Compare December 25, 2022 09:05

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 25, 2022

oshoval approved these changes Dec 25, 2022

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 25, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 25, 2022

kubevirt-bot merged commit d73a470 into kubevirt:main Dec 25, 2022

		@@ -55,10 +56,33 @@ func (zoneFileCache *ZoneFileCache) prepare() {
		zoneFileCache.generateHeaderSuffix()
		zoneFileCache.soaSerial = 0

		zoneFileCache.header = zoneFileCache.generateHeader()
		zoneFileCache.content = zoneFileCache.header


		zoneMgr.zoneFile = NewZoneFile(zoneFileName + zoneMgr.zoneFileCache.domain)
		func (zoneMgr *ZoneManager) initSoaSerial() int {

Handle status monitor container reload #45

Handle status monitor container reload #45

Conversation

dteplits commented Dec 8, 2022 • edited Loading

oshoval left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

AlonaKaplan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dteplits commented Dec 14, 2022 • edited Loading

oshoval commented Dec 14, 2022 • edited Loading

dteplits commented Dec 14, 2022

oshoval commented Dec 14, 2022 • edited Loading

dteplits commented Dec 14, 2022

AlonaKaplan commented Dec 15, 2022

oshoval commented Dec 15, 2022

oshoval Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dteplits commented Dec 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval left a comment

dteplits commented Dec 8, 2022 •

edited

Loading

oshoval left a comment •

edited

Loading

oshoval Dec 8, 2022 •

edited

Loading

oshoval Dec 11, 2022 •

edited

Loading

oshoval Dec 14, 2022 •

edited

Loading

oshoval Dec 18, 2022 •

edited

Loading

oshoval Dec 18, 2022 •

edited

Loading

dteplits commented Dec 14, 2022 •

edited

Loading

oshoval commented Dec 14, 2022 •

edited

Loading

oshoval commented Dec 14, 2022 •

edited

Loading

oshoval Dec 18, 2022 •

edited

Loading

oshoval Dec 21, 2022 •

edited

Loading

oshoval Dec 22, 2022 •

edited

Loading

oshoval Dec 21, 2022 •

edited

Loading